feat(config): accept GOMODEL_-prefixed env vars, deprecate bare names#541
Open
SantiagoDePolonia wants to merge 3 commits into
Open
feat(config): accept GOMODEL_-prefixed env vars, deprecate bare names#541SantiagoDePolonia wants to merge 3 commits into
SantiagoDePolonia wants to merge 3 commits into
Conversation
GoModel-defined environment variables are now canonically spelled GOMODEL_<NAME>. 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, <PROVIDER>_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_<N>). 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) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+116
to
+140
| 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} | ||
| } |
There was a problem hiding this comment.
Scan treats any canonical spelling as authoritative, even when its value is empty. With GOMODEL_SET_RATE_LIMIT_TEAM= and SET_RATE_LIMIT_TEAM=rpm=10, line 134 drops the non-empty legacy value, then applyKeyedLimitEnv skips the empty entry, so the configured limit disappears. This contradicts Lookup and the migration rule that empty canonical values do not shadow working legacy values.
Context Used: CLAUDE.md (source)
…ng 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 <noreply@anthropic.com>
SantiagoDePolonia
force-pushed
the
feat/env-prefix-compat
branch
from
July 17, 2026 13:17
85e40fb to
9b9626d
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR adds a ChangesEnvironment Prefix Migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Process
participant ConfigLoader
participant envcompat
participant slog
Process->>ConfigLoader: Load configuration
ConfigLoader->>envcompat: Resolve GOMODEL or legacy variables
envcompat->>slog: Emit deduplicated legacy warning
envcompat-->>ConfigLoader: Return resolved values
ConfigLoader-->>Process: Apply configuration
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
Follow-up commit
New regression tests cover each behavior (blank-canonical Scan fallthrough, whitespace shadowing, Quiet-then-Get warn-once, prefixed-exempt warning, canonical |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.template:
- Around line 4-5: Update the canonical-versus-legacy precedence documentation
to state that the canonical GOMODEL_ value wins when nonblank, but a blank or
whitespace-only canonical value falls back to a nonblank legacy value; apply
this wording consistently at .env.template lines 4-5 and CLAUDE.md lines
109-110.
In `@internal/envcompat/envcompat.go`:
- Around line 77-86: Update Lookup to detect when a prefixed input strips to an
exempt bare name before the generic prefixed-name branch, and reject it without
reading the environment value. Preserve the existing warning behavior for bare
exempt lookups, and extend TestPrefixedExemptNameWarns to verify the
already-prefixed call form.
In `@run/health.go`:
- Around line 19-29: Make slog suppression scoped to each probe instead of
permanently changing the process-wide logger. Update runHealthProbe and
runReadyProbe to save slog.Default(), install the discard logger, and defer
restoration of the saved logger for the probe’s full execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a9c44fb-978f-4c67-a220-8607c1954ed0
📒 Files selected for processing (22)
.env.templateCLAUDE.mdconfig/cache.goconfig/config.goconfig/config_test.goconfig/env.goconfig/env_prefix_test.goconfig/mcp.goconfig/tagging.goconfig/user_path_env.goconfig/virtualmodels.godocs/dev/2026-07-17_env-prefix-migration.mdinternal/envcompat/envcompat.gointernal/envcompat/envcompat_test.gointernal/httpclient/client.gointernal/providers/anthropic/request_translation.gointernal/providers/gemini/gemini.gointernal/providers/opencodego/opencodego.gointernal/providers/openrouter/openrouter.gointernal/providers/openrouter/openrouter_test.gorun/health.gorun/logging.go
Comment on lines
+4
to
+5
| # they will be removed in a future major release. Setting both spellings | ||
| # resolves to the GOMODEL_ one. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the actual canonical-versus-legacy precedence.
Both summaries say canonical always wins when both spellings exist, but a blank or whitespace-only canonical value falls back to a nonblank legacy value.
.env.template#L4-L5: add the blank-canonical fallback exception.CLAUDE.md#L109-L110: describe the same exception consistently.
📍 Affects 2 files
.env.template#L4-L5(this comment)CLAUDE.md#L109-L110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.template around lines 4 - 5, Update the canonical-versus-legacy
precedence documentation to state that the canonical GOMODEL_ value wins when
nonblank, but a blank or whitespace-only canonical value falls back to a
nonblank legacy value; apply this wording consistently at .env.template lines
4-5 and CLAUDE.md lines 109-110.
Source: Coding guidelines
Comment on lines
+77
to
+86
| 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 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject already-prefixed exempt names too.
Lookup("GOMODEL_PORT") reaches the generic prefixed-name branch and reads the value, contradicting the contract that GOMODEL_PORT is never honored. Check the stripped name against exempt before accepting prefixed inputs, and extend TestPrefixedExemptNameWarns to cover this call form.
Proposed fix
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) {
+ if bare := strings.TrimPrefix(name, Prefix); exempt[bare] {
+ if _, set := os.LookupEnv(name); set && !quiet {
+ warnPrefixedExempt(bare)
+ }
+ return "", false, name
+ }
value, ok = os.LookupEnv(name)
return value, ok, name
}As per coding guidelines, PORT and REDIS_URL are permanently bare variables.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if 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 | |
| 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) { | |
| if bare := strings.TrimPrefix(name, Prefix); exempt[bare] { | |
| if _, set := os.LookupEnv(name); set && !quiet { | |
| warnPrefixedExempt(bare) | |
| } | |
| return "", false, name | |
| } | |
| value, ok = os.LookupEnv(name) | |
| return value, ok, name |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/envcompat/envcompat.go` around lines 77 - 86, Update Lookup to
detect when a prefixed input strips to an exempt bare name before the generic
prefixed-name branch, and reject it without reading the environment value.
Preserve the existing warning behavior for bare exempt lookups, and extend
TestPrefixedExemptNameWarns to verify the already-prefixed call form.
Source: Coding guidelines
Comment on lines
+19
to
+29
| // 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() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore the process-wide logger after each probe.
silenceProbeLogging permanently discards all subsequent slog output in the process, affecting embedded callers and later tests. Save the previous logger and restore it with defer.
Proposed fix
-func silenceProbeLogging() {
+func silenceProbeLogging() func() {
+ previous := slog.Default()
slog.SetDefault(slog.New(slog.DiscardHandler))
+ return func() { slog.SetDefault(previous) }
}
func runHealthProbe(timeout time.Duration) error {
- silenceProbeLogging()
+ restoreLogging := silenceProbeLogging()
+ defer restoreLogging()Apply the same scoped restoration in runReadyProbe.
As per coding guidelines, “Configuration and runtime behavior must preserve stateless request handling and avoid unsafe reliance on hidden global state.”
Also applies to: 45-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@run/health.go` around lines 19 - 29, Make slog suppression scoped to each
probe instead of permanently changing the process-wide logger. Update
runHealthProbe and runReadyProbe to save slog.Default(), install the discard
logger, and defer restoration of the saved logger for the probe’s full
execution.
Source: Coding guidelines
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
GoModel-defined environment variables are now canonically spelled
GOMODEL_<NAME>. The unprefixed spelling still resolves, so no existing deployment breaks — but reading one logs a warning naming its replacement.Why
The environment namespace is global and flat — shared with every other process in the container.
BASE_PATH,HTTP_TIMEOUT,STORAGE_TYPE, andLOGGING_ENABLEDare land grabs on generic names that say nothing about who owns them, and they collide the first time an operator shares anenv_fileacross Compose services or anenvFromConfigMap across containers.This follows the common convention — prefix what you own, go bare only to join someone else's convention. WordPress (
WORDPRESS_DB_HOST), Apache (APACHE_RUN_USER), Grafana (GF_SERVER_HTTP_PORT), Vault (VAULT_ADDR), Ollama (OLLAMA_HOST). LiteLLM makes the same two-tier split:LITELLM_MASTER_KEYfor its own config, bareOPENAI_API_KEYfor the ecosystem.GOMODEL_MASTER_KEYandGOMODEL_CACHE_DIRalready followed the convention. "Everything bare except two" was the worst state — readers couldn't infer the rule either way.What stays bare (permanently)
PORT,REDIS_URLPORT; their Redis plugins setREDIS_URL, so bare = zero-config wiring.<PROVIDER>_API_KEY,<PROVIDER>_BASE_URL,<PROVIDER>_MODELS, ...OPENAI_API_KEYis the OpenAI SDK's own name — reading it bare is what makes GoModel drop-in. Generated from provider name + suffix, so it moves as one unit or not at all.DOCKER_HOST,MOCK_*,RECORD,UPDATE_GOLDEN,TEST_*POSTGRES_URL/MONGODB_URLare not exempt despite looking like ecosystem names — nothing injects them. The real convention isDATABASE_URL, which GoModel doesn't read today; accepting it would be a separate feature, not a rename.Design notes for review
internal/envcompatand notconfig/:HTTP_TIMEOUTis read twice — by the config struct tags and independently byinternal/httpclient, which can't importconfig.GOMODEL_SQLITE_PATH=in a compose file alongside a realSQLITE_PATHresolves to the real one instead of silently discarding it. Presence is still reported when only empty values are set, sook-bool callers (RESPONSE_CACHE_SIMPLE_ENABLED=means "off") keep working.os.GetenvinapplyEnvOverridesValue. A prefix scan covers the four families discovered by walkingos.Environ():SET_BUDGET_*,SET_RATE_LIMIT_*,SET_PROVIDER_RATE_LIMIT_*,TAGGING_HEADER_<N>.Scansorts by suffix. Callers resolve suffixes to canonical keys and two suffixes can collide there (SET_RATE_LIMIT_A_andSET_RATE_LIMIT_Aboth name patha), so iteration order decided the winner — previously dependent on OS ordering.Scanreports the legacy spelling, so companions resolve back throughenvcompat. This is what lets a canonicalGOMODEL_TAGGING_HEADER_1pair with a legacyTAGGING_HEADER_1_PREFIX.GOMODEL_CONFIG_STRICTgoverns the YAML parse and runs before the tag walker, so it callsenvcompatat its own site.LOG_LEVEL/LOG_FORMATare included — they live inrun/rather thanconfig/and were missed by the original survey.Verification
Beyond unit tests, run against the real binary with mixed spellings:
/health→ 200 with barePORT=18099(exempt path intact)GOMODEL_SQLITE_PATHcreated the DB at the canonical path (prefix drives real behavior)GOMODEL_LOG_FORMAT=jsontook effectLOGGING_LOG_BODIESwarned; canonical vars stayed quietmake test-race,make lint, and the hot-path alloc guard all pass.Not in this PR
Documentation still shows the legacy spellings. Renaming that surface (
.env.template,config.example.yaml,README,helm/, compose files) is a large mechanical diff that the exempt rules above make unsafe to do blindly — it's a follow-up..env.template,CLAUDE.md, anddocs/dev/2026-07-17_env-prefix-migration.mddocument the convention and the full mapping in the meantime.🤖 Generated with Claude Code
Summary by CodeRabbit
GOMODEL_environment-variable names.PORT,REDIS_URL, and provider-scoped variables (with special-case handling where applicable).