feat!: reject unrecognized model_config settings and bump coder/coder to latest main#388
Conversation
…ently drop Add a plan-time validator for coderd_agents_model.model_config that canonicalizes the user's JSON through codersdk.ChatModelCallConfig and compares object key names before and after. Any key present in the input but gone after the round-trip is a setting Coder does not recognize and would silently discard, so the validator raises a plan-time error naming those keys instead of losing configuration with no diff. json.DisallowUnknownFields cannot be used here because the SDK's custom UnmarshalJSON calls json.Unmarshal internally, which defeats strict decoding for the whole subtree.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6602821b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Compare model_config key paths instead of bare names so a valid key can't mask a dropped occurrence of the same name elsewhere, with an exception for the SDK's legacy top-level pricing keys folded under cost. Also drops the unused sliceContains helper that failed lint.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7934eebbec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 986c469266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bc9c6aca5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ff5a42293
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ethanndickson
changed the title
ethanndickson
changed the title
…eys instead of hardcoding schema
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
I feel like this adds a lot of complexity for not much gain.
Do we plan eventually to promote the model config into a top-level Terraform type? Would it be simpler to just promote once it stabilizes?
| } | ||
| // Invalid or non-object JSON is left for the custom type and the not-empty | ||
| // validator to report. | ||
| dropped, err := agentsModelConfigDroppedKeys(req.ConfigValue.ValueString()) |
There was a problem hiding this comment.
Would it be simpler to just marshal to the SDK config type and back, and assert JSON equivalence? (leaving out the problems of ordering and whitespace)
Yeah I think that's a reasonable criticism. The reason I'm doing this is because Mike and his agent had no idea his coder/dogfood PR to adapt the thinking levels wasn't going to do anything until the codex review bot flagged the codersdk on this repo needed bumping, but I think this applies to customers too. People pin their Terraform providers, and without this they might upgrade coder, but forgot to update the provider, and be left wondering why their config didn't apply. The provider wouldn't tell them without this.
No plans to promote it unfortunately, when I looked at doing |
Fair point. In that case, I wonder if it would make sense to move the validation into |
Ah good shout. Yeah we could just add an |
…ropped model_config settings Replaces the removal-probe drop detection with codersdk.ChatModelCallConfig.UnmarshalStrict (coder/coder#27187), pinning the SDK to that PR's commit. The strict decoder shares the lenient path's aux struct, so the SDK stays the sole oracle with no schema knowledge in the provider.
| "model_config contains a setting that is not part of the Coder chat model config schema this provider was built against, so it would be silently dropped before reaching Coder: %s. "+ | ||
| "If the setting is valid for your Coder deployment, upgrade the provider to a version built against your Coder release; otherwise remove it or fix the field name. "+ |
There was a problem hiding this comment.
non-blocking: As an end-user, how do you determine this? Something to think about in future, I suppose.
There was a problem hiding this comment.
ask an agent
Yeah, not sure... Bit of an experimental-API only paper cut... We haven't had to ask users to pay attention to the provider versions like this in the past.
…xtures to top-level reasoning_effort Folds the non-go.mod changes from #386 into this branch, since it already pins an SDK with the reasoning_effort restructure.
ethanndickson
changed the title
|
coder/dogfood#397 is now green and waiting on a provider release containing this SDK bump. Its current provider pin silently drops top-level
|
|
The v0.0.22 GitHub release completed successfully, but Terraform Registry has not indexed v0.0.21 or v0.0.22. The Registry provider metadata is still Could a maintainer check or resync the Terraform Registry publication? coder/dogfood#397 is ready to consume v0.0.22 once the Registry serves it.
|
Yeah I'm resyncing now |
3 participants
Closes CODAGT-797
Adds a plan-time validator for
coderd_agents_model.model_configthat fails loudly when the config contains settings the provider would silently drop when decoding into its pinned Coder SDK.Why
Each provider release pins exactly one
codersdk, andmodel_configis decoded into that SDK'sChatModelCallConfigbefore every create/update — so a release supports exactly onemodel_configschema. Standardencoding/jsonignores object keys with no matching struct field, so anything outside that schema (a misspelled key, a field from a newer Coder, a field removed or renamed by an SDK bump) is silently dropped in the provider before the request is ever sent, with no plan diff, and the value in state compares semantically equal. A user can lose configured behavior with no signal.What
agentsModelConfigNoDroppedKeysValidator, wired into themodel_configschema alongside the existing not-empty validator.provider_options.anthropic.bogus_setting), so a name that is valid in one place cannot mask a dropped occurrence of the same name elsewhere.This is schema-change-agnostic: nothing in the implementation restates the SDK schema, so it cannot drift when
ChatModelCallConfigevolves.Design notes
ChatModelCallConfig, and compares against the canonical output of the full document: a key whose removal leaves the output unchanged contributed nothing, so it was dropped. Nothing hardcodes recognized fields, legacy aliases, or folding rules.cost) probes as kept, because removing it changes the output. The same key shadowed by a differingcost.<key>(whose nested value wins) probes as dropped, because removing the ignored legacy value changes nothing — so silent overrides are still caught.null,[], and{}hitomitemptywhether or not the key is recognized, so removing one never changes the output; the probe cannot judge them and does not try.json.DisallowUnknownFieldsdoes not work here:ChatModelCallConfighas a customUnmarshalJSONthat calls plainjson.Unmarshalinternally, discarding the decoder's strict setting for the whole subtree.Compatibility model
The supported way to consume this (experimental) resource is to run the provider release that matches your Coder version; cross-version compatibility is delivered by the release stream (see #386's release plan), not by any single provider binary.
We considered making
model_configan opaque passthrough blob (raw JSON both ways, bypassing the typed SDK client) so one provider release could span Coder versions, and rejected it as counter to that compatibility story. Passthrough would let this one attribute float across versions while everything else about the resource stays typed to the pinned SDK — a false sense of cross-version support — and it silently accepts typos and unsupported settings, which match no Coder version, get dropped by the server on write, and never come back: a perpetual diff with no error. This validator enforces the contract loudly instead: any setting outside the schema the provider was built against fails at plan time, with the remedy in the diagnostic (fix the field name, or upgrade the provider if the setting is valid on your Coder). Combining passthrough with this validation wouldn't help either — the error rejects exactly the newer-server fields passthrough exists to forward, and softening it to a warning just re-introduces quiet drift while adding the raw-HTTP machinery passthrough requires.The validator checks against the pinned SDK, not the live server, so the reverse mismatch — a provider newer than your Coder — can still send fields the server drops; that too is solved by matching versions.
Opened by Coder Agents on behalf of @ethanndickson.