fix(config): reject unknown config keys and log provider origins#511
Conversation
A misindented `providers:` section parses as a null section plus unknown top-level keys. yaml.Unmarshal discarded those silently, so the gateway booted with none of the operator's providers and no error — the fault only surfaced once an env-provisioned provider was removed and startup failed with "no providers were successfully registered". Parse the YAML layer with KnownFields(true) so an unknown key fails startup, naming the file and every offending line. This also catches ordinary typos (`prot:` for `port:`, `bass_url:` for `base_url:`). The env layer declares the same structures as JSON and overrides YAML entry by entry, so a typo there would silently win over a correct YAML entry. Decode VIRTUAL_MODELS, SET_RATE_LIMIT_*, and SET_BUDGET_* with DisallowUnknownFields for the same reason. Two fixes fall out of that: - BudgetLimitConfig had no json tags, unlike its RateLimitRuleConfig neighbour which added them for exactly this env form, so `period_seconds` in a SET_BUDGET_* JSON array was silently dropped and produced a limit with no window. Add the tags. - `failover.overrides` is a removed key that old config files still carry and that strict parsing would now reject. Model it as an explicitly ignored field, upgrading it from silently ignored to a logged deprecation pointing at `disabled_models`. A config path that exists but cannot be read — a directory bind-mounted where a file was expected — was indistinguishable from a missing file and fell back to defaults. Report it instead. Finally, log one line at boot showing how many providers came from the config file versus environment discovery, with their names, so an operator can see at a glance that a config file contributed nothing. Closes #509 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 (4)
📝 WalkthroughWalkthroughThis PR adds strict validation for YAML config and JSON env inputs, introduces ChangesStrict config validation and provider origin logging
Go toolchain version bump
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
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}
| func decodeStrictJSON(raw string, target any) error { | ||
| decoder := json.NewDecoder(strings.NewReader(raw)) | ||
| decoder.DisallowUnknownFields() | ||
| return decoder.Decode(target) | ||
| } |
There was a problem hiding this comment.
Decode succeeds after the first JSON value and leaves trailing bytes unread, so VIRTUAL_MODELS='[{"source":"a"}] {"targts":[]}' or SET_RATE_LIMIT_X='[{"period":"minute","max_requests":1}] junk' boots successfully while ignoring the malformed suffix. This regresses the previous json.Unmarshal behavior and weakens the new strict env validation; decode a second token and require io.EOF after the first value.
Artifacts
Repro: targeted Go test for trailing JSON acceptance
- Contains supporting evidence from the run (text/x-go; charset=utf-8).
Repro: verbose go test output showing trailing malformed JSON accepted
- Keeps the command output available without making the summary code-heavy.
Strict parsing is the right default — a dropped providers, rate_limits, budgets, or guardrails entry silently changes routing, cost, or security — but it blocks one legitimate case: rolling a binary back under a config file written for a newer version, where an unknown key is expected rather than a typo. CONFIG_STRICT=false downgrades unknown keys to warnings that name the file, line, and field, then boots without them. The default stays true. Unknown keys are always detected: lax mode still decodes with KnownFields(true) and inspects the resulting TypeError, so it can report each ignored key instead of dropping it in silence the way yaml.Unmarshal did. The flag relaxes which keys are accepted, never whether a value makes sense. A malformed value (`port: [9999, 8080]`) and a YAML syntax error stay fatal in both modes, and a file mixing an unknown key with a malformed value still fails. The same rule applies to the JSON env layer, where only encoding/json's unknown-field error is downgraded. Entity-level validation is unaffected: a virtual model whose target names a provider that does not exist still fails startup even when the key that would have declared that provider was ignored with a warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/ratelimit_test.go (1)
410-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a lax-mode test case and make this table-driven.
The test only covers
strict=true. Astrict=falsecase verifying that unknown fields are downgraded to warnings (and parsing succeeds) would close the behavior-change test gap. As per coding guidelines,**/*_test.gofiles should use table-driven tests.♻️ Proposed table-driven test with strict and lax cases
func TestParseRateLimitEnvLimits_RejectsUnknownField(t *testing.T) { - _, err := parseRateLimitEnvLimits(`[{"period":"minute","max_requsts":100}]`, true) - if err == nil { - t.Fatal("parseRateLimitEnvLimits() error = nil, want unknown-field error") - } - if !strings.Contains(err.Error(), "max_requsts") { - t.Fatalf("parseRateLimitEnvLimits() error = %q, want it to name the unknown field", err) - } + tests := []struct { + name string + strict bool + wantErr bool + }{ + { + name: "strict rejects unknown field", + strict: true, + wantErr: true, + }, + { + name: "lax downgrades unknown field to warning", + strict: false, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + limits, err := parseRateLimitEnvLimits(`[{"period":"minute","max_requsts":100}]`, tt.strict) + if tt.wantErr { + if err == nil { + t.Fatal("parseRateLimitEnvLimits() error = nil, want unknown-field error") + } + if !strings.Contains(err.Error(), "max_requsts") { + t.Fatalf("parseRateLimitEnvLimits() error = %q, want it to name the unknown field", err) + } + } else { + if err != nil { + t.Fatalf("parseRateLimitEnvLimits() error = %v, want nil (lax mode)", err) + } + if len(limits) != 1 || limits[0].Period != "minute" { + t.Fatalf("parseRateLimitEnvLimits() limits = %v, want one rule with period minute", limits) + } + } + }) + } }🤖 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 `@config/ratelimit_test.go` around lines 410 - 418, The test for parseRateLimitEnvLimits currently only checks strict mode and should be converted into a table-driven test in TestParseRateLimitEnvLimits_RejectsUnknownField. Add cases for both strict=true and strict=false, using the existing parseRateLimitEnvLimits helper and verifying that unknown fields still error in strict mode but are accepted as warnings in lax mode. Keep the assertions focused on the unknown field name and the success path, and structure the cases so the behavior difference is explicit and easy to extend.Source: Coding guidelines
🤖 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 `@config/merge.go`:
- Around line 14-29: decodeStrictJSON currently accepts extra trailing content
because Decoder.Decode only reads the first JSON value, so add an explicit
end-of-input verification after the initial decode in decodeStrictJSON. Use the
existing decodeStrictJSON helper (and its json.Decoder setup) to ensure inputs
like valid JSON followed by junk are rejected, and avoid using Decoder.More()
for this top-level check; keep decodeIaCJSON unchanged except for relying on the
stricter behavior.
In `@docs/advanced/config-yaml.mdx`:
- Around line 176-177: The warning count in the config example is inconsistent
with the single unknown YAML key shown in the earlier sample. Update the
sentence in the docs so the behavior described by the CONFIG_STRICT=false case
matches the one unrecognized top-level key in the YAML example, and reference
the surrounding CONFIG_STRICT explanation in config-yaml.mdx to keep the
strict/lax parsing description aligned.
---
Outside diff comments:
In `@config/ratelimit_test.go`:
- Around line 410-418: The test for parseRateLimitEnvLimits currently only
checks strict mode and should be converted into a table-driven test in
TestParseRateLimitEnvLimits_RejectsUnknownField. Add cases for both strict=true
and strict=false, using the existing parseRateLimitEnvLimits helper and
verifying that unknown fields still error in strict mode but are accepted as
warnings in lax mode. Keep the assertions focused on the unknown field name and
the success path, and structure the cases so the behavior difference is explicit
and easy to extend.
🪄 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: b7f4615b-c4c7-464a-97ef-4cce9a80569c
📒 Files selected for processing (12)
.env.templateconfig/budget.goconfig/budget_test.goconfig/config.goconfig/config_strict_test.goconfig/config_test.goconfig/merge.goconfig/ratelimit.goconfig/ratelimit_test.goconfig/virtualmodels.goconfig/virtualmodels_test.godocs/advanced/config-yaml.mdx
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Swapping json.Unmarshal for a json.Decoder to get DisallowUnknownFields
regressed a check nobody asked for but everybody relied on: Decode stops
after the first JSON value and leaves the rest unread, so
VIRTUAL_MODELS='[{"source":"a"}] {"targts":[]}'
booted happily while ignoring the suffix. json.Unmarshal rejects trailing
data; silently applying half an env var is precisely the failure this path
exists to prevent. Require io.EOF after the decoded value.
The YAML layer had the same hole, pre-dating this branch: both yaml.Unmarshal
and yaml.Decoder read one document, so every key after a `---` separator was
applied nowhere. Reject a second document.
Both are structural faults, not unknown keys, so they stay fatal under
CONFIG_STRICT=false.
Also correct the misindentation gotcha in the docs, whose prose counted keys
the sample did not contain. The sample now carries two providers and the
quoted error is the binary's verbatim output.
Reported by Greptile and CodeRabbit on #511.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed both bot findings. Trailing JSON (P1 / Major — @greptile-apps, @coderabbitai). Confirmed and fixed. Swapping
The same hole existed in the YAML layer, pre-dating this branch and unflagged: Docs (Minor — @coderabbitai). Correct: the prose counted keys the sample did not contain. The sample now carries two providers, and the quoted error string is the binary's verbatim output rather than my paraphrase. Verified with unit, contract, e2e, and Docker-backed integration suites, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
fd . -t f config/merge.go config/config.go
echo "--- merge.go ---"
cat -n config/merge.go
echo "--- config.go (grep decoder/EOF) ---"
rg -n "NewDecoder|DisallowUnknownFields|io.EOF|decodeStrictJSON" config/config.go config/merge.goLength of output: 724 🏁 Script executed: #!/bin/bash
set -e
fd merge.go
fd config.go
echo "--- merge.go ---"
fd merge.go --exec cat -n {}
echo "--- config.go (grep decoder/EOF) ---"
fd '^(merge|config)\.go$' -p config --exec rg -n "NewDecoder|DisallowUnknownFields|io.EOF|decodeStrictJSON" {}Length of output: 10211 🏁 Script executed: #!/bin/bash
set -e
CFG=$(fd '^config\.go$' config)
echo "$CFG"
rg -n "NewDecoder|DisallowUnknownFields|io.EOF|multi.?doc|---" "$CFG" | head -50Length of output: 686 Verified—the fixes described in your comment are present in the current code. Nice catch turning Greptile's repro into full coverage across both the JSON and YAML layers — that's exactly the kind of regression that's easy to miss when swapping ✏️ Learnings added
|
|
Both fixes look solid. Trailing JSON: Docs: The misindented sample now has two providers, and the error string is byte-for-byte what the binary emits. The LGTM on both. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/advanced/config-yaml.mdx (1)
88-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winJSON log example:
from_config_filecount doesn't match the provider list.
from_config_file=4andtotal=5, butconfig_file_providerslists only 2 names. An operator using this example to understand the log format would expect the array length to match the count.📝 Suggested fix
{"msg":"providers resolved","total":5,"from_config_file":4,"from_env":1, - "config_file_providers":["plutonium-qwen-dflash","uranium-gemma-embedding"], + "config_file_providers":["plutonium-qwen-dflash","uranium-gemma-embedding","neptunium-llama-vanadium","kryptonite-mistral-fusion"], "env_providers":["opencode-go"]}🤖 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 `@docs/advanced/config-yaml.mdx` around lines 88 - 92, The JSON log example for the “providers resolved” message has inconsistent counts, since from_config_file says 4 while config_file_providers only lists 2 entries. Update the example values so total, from_config_file, env, and the provider arrays all agree, keeping the log shape in sync with the intended format in the config YAML docs.
🤖 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.
Outside diff comments:
In `@docs/advanced/config-yaml.mdx`:
- Around line 88-92: The JSON log example for the “providers resolved” message
has inconsistent counts, since from_config_file says 4 while
config_file_providers only lists 2 entries. Update the example values so total,
from_config_file, env, and the provider arrays all agree, keeping the log shape
in sync with the intended format in the config YAML docs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8bb1223-4136-48fd-91fe-73363eb74659
📒 Files selected for processing (5)
config/config.goconfig/config_strict_test.goconfig/merge.goconfig/virtualmodels_test.godocs/advanced/config-yaml.mdx
govulncheck reports the Encrypted Client Hello privacy leak in crypto/tls (GO-2026-5856), fixed in go1.26.5. It reproduces on pristine main at e0eb0ee, so CI is red independently of this branch; the reachable traces are the dashboard, redis, and mongo TLS paths, none of them new. Bump every pin so the shipped binary is patched, not just CI: .github/workflows/test.yml GO_VERSION 1.26.4 -> 1.26.5 .github/workflows/release.yml go-version 1.26.4 -> 1.26.5 Dockerfile golang:1.26.4-alpine3.23 -> 1.26.5 go.mod go 1.26.4 -> go 1.26.5 Verified: govulncheck reports no vulnerabilities with and without the swagger tag; unit, contract, e2e, and integration suites pass; golangci-lint is clean across all build tags; and the release image builds and reports go1.26.5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Note: this PR now also bumps Go to 1.26.5Unrelated to the config changes, folded in at the author's request because it blocks CI.
The reachable traces are the existing dashboard, redis, and mongo TLS paths. Bumped every pin, so the shipped binary is patched rather than just CI:
Verified locally on go1.26.5: If you would rather keep this PR focused, the last commit ( |
Conflicts were confined to additive blocks landing in the same locations: app.go (MCP gateway init vs main's shared usage reader — kept both), handlers.go imports and Handler fields (mcpgateway vs realtime WebRTC — kept both), and the handler methods block (MCP endpoints vs realtime calls/client_secrets — kept both). Swagger docs regenerated on the merged tree instead of trusting the auto-merge of generated files, and go fix applied to clear pre-existing drift on main (keyring_test.go) that the new fix-check pre-commit hook flags. Verified the mcp: config block passes main's new strict unknown-key validation (#511). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Closes #509
Problem
A misindented
providers:section is valid YAML — just not the YAML you meant:It parses as
providers: nullplus four unrelated top-level keys, whichyaml.Unmarshaldiscarded silently. The gateway booted with none of the operator's providers and no error. In #509 the fault stayed hidden because an env-provisioned OpenCode provider kept the registry non-empty; removing it turned the silent misconfiguration intono providers were successfully registered, which points nowhere near the real cause.User-visible impact
Unknown keys in declarative config now fail startup by default, naming the file and every offending line:
This also catches ordinary typos (
prot:forport:,bass_url:forbase_url:).A boot log line reports where providers came from, as requested in #509:
{"msg":"config file loaded","path":"config.yaml","providers":4} {"msg":"providers resolved","total":5,"from_config_file":4,"from_env":1, "config_file_providers":["plutonium-qwen-dflash","uranium-geryon-9b"], "env_providers":["opencode-go"]}from_config_file: 0while a file is loaded means the file contributed nothing. There is alsono config file foundlisting the searched paths.CONFIG_STRICT=falseescape hatchStrict is the right default: a dropped
providers,rate_limits,budgets, orguardrailsentry silently changes routing, cost, or security — a cost cap that never applies looks exactly like a cost cap that does. Strict's worst case is a boot failure with a precise message; liberal's worst case is a gateway running for weeks with a control silently absent.But it blocks one legitimate case: rolling a binary back under a config file written for a newer version, where an unknown key is expected rather than a typo.
CONFIG_STRICT=falsedowngrades unknown keys to warnings and boots without them:Three properties make the lax mode meaningfully better than the old silent behaviour:
KnownFields(true)and inspects the resultingTypeError, so it names each ignored key rather than dropping it in silence the wayyaml.Unmarshaldid.port: [9999, 8080]) or a YAML syntax error stays fatal in both modes, and a file mixing an unknown key with a malformed value still fails.Why the env layer changed too
Per the documented priority (env > yaml > defaults), env entries override YAML entries key by key. Leaving env JSON lax while YAML is strict recreates the same trap one layer up: a typo in
VIRTUAL_MODELS,SET_RATE_LIMIT_*, orSET_BUDGET_*would silently win over a correct YAML entry. All three now decode through one shared helper that honoursCONFIG_STRICTand downgrades onlyencoding/json's unknown-field error.Two things fall out of that:
BudgetLimitConfighad no json tags, unlike itsRateLimitRuleConfigneighbour which added them for exactly this env form.SET_BUDGET_X='[{"period_seconds":7200,"amount":5}]'silently droppedperiod_seconds, producing a limit with no window. Verified against the old code, then fixed by adding the tags.failover.overrides. A removed key that old config files still carry, and that existing tests deliberately assert still loads. Strict parsing would have broken that contract, so it is modelled as an explicitly ignored field — upgrading it from silently ignored to a logged deprecation pointing atdisabled_models.Also fixed
A config path that exists but cannot be read — most often a directory bind-mounted where a file was expected, a common Docker mistake — was indistinguishable from a missing file and silently fell back to defaults. It is now reported:
failed to read config.yaml: read config.yaml: is a directory.Breaking change
With the default
CONFIG_STRICT=true, configs carrying stray keys will not boot. The error names the key and the line, andCONFIG_STRICT=falseis the documented out. Top-level YAML anchors (x-defaults: &d) are also rejected under the default — nothing in the repo or docs uses them, and anchors defined insideproviders:still work.Verification
Full suite passes;
golangci-lintclean; all pre-commit hooks (includingmake test-race) pass. Beyond unit tests, the built binary was driven through every path against the exact config from #509:CONFIG_STRICT=falsefrom_config_file: 0CONFIG_STRICT=false+port: [9999, 8080]CONFIG_STRICT=maybeconfig.yamlNew tests cover misindentation, nested and per-provider typos, empty and comments-only files, the
...document-end marker the reporter's file uses, the unreadable-path case, both strict and lax modes in YAML and env JSON, type errors staying fatal when lax, the budget json-tag fix, and a regression test thatconfig.example.yamlitself parses strictly — otherwise anyone copying it could not boot.🤖 Generated with Claude Code
Summary by CodeRabbit
config.yamland structured JSON-style environment settings are rejected by default (malformed values remain fatal); multi-document YAML is rejected.CONFIG_STRICT(default:true) controls whether unknown keys are rejected or downgraded to warnings.providers.