feat: add experimental coderd_agents_model resource#371
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ethanndickson
changed the title
ethanndickson
changed the base branch from
docs/add-agents-md
to
graphite-base/371
ethanndickson
force-pushed
the
ethan/agents-model-resource
branch
from
1d93af4 to
a40ebf9
Compare
ethanndickson
force-pushed
the
graphite-base/371
branch
from
3db900a to
392562f
Compare
ethanndickson
changed the base branch from
graphite-base/371
to
docs/add-agents-md
ethanndickson
force-pushed
the
ethan/agents-model-resource
branch
2 times, most recently
from
3eba546 to
44955ac
Compare
ethanndickson
force-pushed
the
docs/add-agents-md
branch
2 times, most recently
from
ce7bd51 to
ac6b0b1
Compare
ethanndickson
force-pushed
the
ethan/agents-model-resource
branch
2 times, most recently
from
342a9f1 to
b5bcd68
Compare
ethanndickson
force-pushed
the
docs/add-agents-md
branch
from
ac6b0b1 to
44cd01d
Compare
ethanndickson
force-pushed
the
ethan/agents-model-resource
branch
from
b5bcd68 to
043dfff
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 18 findings (6 P2, 7 P3, 1 P4, 4 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledged(none) CRF-14 (Nit, agents_model_resource.go:150) - Magic number 70
CRF-17 (Nit, agents_model_resource_test.go:624) - testCheckAgentsModelConfig checks only 2 of 4 fields
Law analysisEffective LOC: 1435 additions. Head SHA: 043dfff. Verdict: Don't split. Enforcement: Advisory (not applicable). Round logRound 1Panel. Netero: 2 P3. Law: Don't split. Panel: 14 reviewers (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Ging-Go, Meruem, Chopper, Zoro, Knov, Komugi, Kite). 6 P2, 4 P3, 4 Nit posted. 4 dropped. Reviewed against 44cd01d..043dfff. Round 2Panel. Netero: 0 new findings. Panel: 4 reviewers (Bisky, Mafuuu, Mafu-san, Kite). CRF-14, CRF-17 contested and panel-closed (4/4 accept). 1 P3, 1 P4 new. 14 prior findings verified fixed. Reviewed against 44cd01d..9a02e24. Round 3Netero: 0 findings. CRF-21, CRF-22 addressed in 4353f7a. All 22 findings resolved. Approved. Reviewed against 44cd01d..4353f7a. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![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: 043dfffd18
ℹ️ 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".
ethanndickson
force-pushed
the
ethan/agents-model-resource
branch
from
043dfff to
0179060
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0179060d00
ℹ️ 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".
ethanndickson
force-pushed
the
ethan/agents-model-resource
branch
from
0179060 to
9db9cd2
Compare
|
@codex review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured resource with some genuinely good engineering. The custom agentsModelConfigType that canonicalizes through the SDK struct to prevent perpetual diffs from decimal normalization is the right approach, and the agentsModelConfigNotEmptyValidator turns a confusing Terraform core error into an actionable plan-time message. The useStateForUnknownUnlessChanged plan modifier is clean generic infrastructure. Test coverage is thorough, especially the semantic equality suite and the no-drift acceptance test.
Severity counts: 6 P2, 6 P3, 4 Nit.
The P2s cluster around two themes: (1) Optional+Computed fields with no Default sending explicit zero values to the server when the user omits them (is_default, display_name), and (2) missing test coverage for validation paths that carry real user-facing value. Both are fixable without structural changes.
"If someone deleted every
if !m.X.Equal(state.X)guard and unconditionally set all fields, this test would still pass." (Bisky, on the updateRequest test)
This is a first-round review. The full panel has reviewed.
🤖 This review was automatically generated with Coder Agents.
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
|
/coder-agents-review |
There was a problem hiding this comment.
All 14 prior findings addressed. The fixes are thorough: CRF-3 (is_default omission) guards with IsUnknown(), CRF-4 (list 404) removes the isNotFound guard, CRF-7 (display_name) adds LengthAtLeast(1) + DisplayName validator, CRF-8 drops UseStateForUnknown from is_default AND adds null/unknown guards in updateRequest. CRF-15 went beyond the ask, replacing ImportStateVerifyIgnore with a semantic comparison.
Contested findings CRF-14 (magic number 70) and CRF-17 (testCheckAgentsModelConfig field coverage) both closed by the panel (4/4 accept). The author's defenses are factual: inline literals are house style, and cost round-tripping is proven end-to-end by the no-drift test.
Two new findings, both test coverage gaps for the is_default unknown-value guards introduced by the CRF-3/CRF-8 fixes.
"types.BoolUnknown().ValueBoolPointer() returns &false (unknown has zero-value internals). Dropping the IsUnknown() predicate would silently send is_default: false to the API on every unrelated update, demoting the model from default." (Bisky)
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
|
/coder-agents-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4353f7a08e
ℹ️ 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".
There was a problem hiding this comment.
All findings resolved across three rounds (6 P2, 6 P3, 1 P4, 4 Nit raised; 16 fixed, 2 panel-closed, 4 dropped). The fixes were thorough and several went beyond the minimum: CRF-15 replaced ImportStateVerifyIgnore with semantic comparison, CRF-8 addressed both plan and apply sides, CRF-21/CRF-22 added targeted unit tests for the unknown-value guards.
The resource is well-engineered. The custom agentsModelConfigType that canonicalizes through the SDK struct, the agentsModelConfigNotEmptyValidator that turns a confusing core error into an actionable message, and the useStateForUnknownUnlessChanged plan modifier are all clean, tested, and correctly motivated.
"The model_config custom type shows genuine problem understanding." (Mafu-san, R1)
🤖 This review was automatically generated with Coder Agents.
matifali
left a comment
matifali
left a comment
There was a problem hiding this comment.
Excellent work Ethan. I guess the next step now is to start dogfooding it.
| require.NoError(t, err) | ||
| assert.JSONEq(t, w.config, string(got), "model_config for %s", m.Model) |
There was a problem hiding this comment.
suggest using cmp.Diff instead for nicer diffs
| { | ||
| // Create bound to the anthropic provider. | ||
| Config: cfg(anthropic.ID.String(), 200000), | ||
| Check: resource.TestCheckResourceAttr("coderd_agents_model.sonnet", "provider_type", "anthropic"), | ||
| }, |
There was a problem hiding this comment.
Worthwhile adding an idempotence test?
| "claude-opus-4-8": {"anthropic", `{"max_output_tokens":128000,"cost":{"input_price_per_million_tokens":"5","output_price_per_million_tokens":"25","cache_read_price_per_million_tokens":"0.5","cache_write_price_per_million_tokens":"6.25"},"provider_options":{"anthropic":{"send_reasoning":true,"effort":"high"}}}`}, | ||
| "claude-sonnet-4-6": {"anthropic", `{"cost":{"input_price_per_million_tokens":"3","output_price_per_million_tokens":"15"},"provider_options":{"anthropic":{"send_reasoning":true,"effort":"max","web_search_enabled":true,"thinking":{"budget_tokens":16000}}}}`}, | ||
| "gpt-5.5": {"openai", `{"cost":{"input_price_per_million_tokens":"2.5","output_price_per_million_tokens":"15","cache_read_price_per_million_tokens":"0.25"},"provider_options":{"openai":{"parallel_tool_calls":false,"reasoning_effort":"xhigh","reasoning_summary":"detailed","text_verbosity":"high","web_search_enabled":true,"search_context_size":"medium"}}}`}, | ||
| "gpt-5.4-mini": {"openai", `{"provider_options":{"openai":{"reasoning_effort":"medium"}}}`}, |
There was a problem hiding this comment.
nit: pretty-format for readability
Add a Terraform resource for managing Coder Agents admin-managed chat
model configurations. A model config binds a model identifier to a
configured AI provider via a required ai_provider_id and carries
context/compression settings plus an open-ended per-call tuning blob.
The model_config attribute is a custom normalized JSON string type so it
is passed through to Coder verbatim and compared with JSON semantic
equality, avoiding schema churn as Coder adds tuning fields. An empty
config (jsonencode({})) is rejected at plan time because Coder discards
it, which would otherwise leave Terraform's state inconsistent. Includes
full CRUD, import, examples, generated docs, and focused
unit/acceptance tests.
ethanndickson
force-pushed
the
ethan/agents-model-resource
branch
from
86ac6f2 to
877f8dc
Compare
ethanndickson
force-pushed
the
docs/add-agents-md
branch
from
44cd01d to
13feeb1
Compare
3 participants
Relates to CODAGT-607
Summary
Adds
coderd_agents_modelfor managing Coder Agents admin-managed chat model configurations from Terraform — binding a model identifier to a configured AI provider — with full CRUD + import, generated docs/examples, acceptance/unit tests and an integration test with the provider resource. The resource is marked experimental via a warning callout in its docs.Approach
model_configas normalized JSON. The open-ended per-call tuning blob (token limits, temperature, cost/pricing, provider options) is a single JSON string backed by a custom type that compares by JSON semantic equality, so whitespace, key ordering, and equivalent number spellings (a3.00cost returned as3) don't produce perpetual diffs. An emptyjsonencode({})is rejected at plan time because Coder discards it.provider_options. Common per-call knobs — reasoning effort, reasoning summary, parallel tool calls, and web search — are nested per provider rather than top-level, since each provider names them differently. Anthropic useseffort(and/orthinking.budget_tokens),send_reasoning,web_search_enabled, anddisable_parallel_tool_use; OpenAI usesreasoning_effort,reasoning_summary,parallel_tool_calls, andweb_search_enabled. The examples show both.provider_typederived fromai_provider_id. A model takes its provider via a requiredai_provider_id, and Coder derives the runtimeprovider_typefrom it. A plan modifier keeps the prior value whileai_provider_idis unchanged and lets it recompute when the binding changes, so it never shows stale plan noise or trips the post-apply consistency check.is_defaultis optional/computed: set it totrueon one model and omit it on others. Settingfalseis rejected by config validation, since the server manages promotion/demotion itself and would otherwise produce a post-apply consistency error.Why
model_configis JSON, not typed attributesprovider_optionsis a tagged union (exactly one ofopenai/anthropic/google/… with non-obvious aliases like Bedrock→anthropic), and the config carriesdecimalpricing and free-formmap[string]anyfields — none of which Terraform's type system can express without re-implementing the server's own validation. A single normalized JSON string sidesteps all of that and lets new upstream tuning fields land with acodersdkbump instead of schema churn. This mirrors the closest prior art: AWS'sbedrockagentadditional_model_request_fields, which models per-provider model overrides the same way.Schema
Computed:
id,provider_type(derived fromai_provider_id),display_name,enabled,is_default,compression_threshold,created_at, andupdated_at.Notes
No write-only arguments, so there's no Terraform 1.11+ requirement — the API key lives on the referenced
coderd_ai_provider. The chat-model endpoints are experimental (/api/experimental); since there's no get-by-id route, Read lists model configs and filters by ID.Follow-up to #368 (
coderd_ai_provider).