feat: add experimental coderd_default_agents_model resource#378
Conversation
|
@codex review |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 8 findings (4 P3, 1 P4, 3 Nit), APPROVE. Review Finding inventoryFinding inventoryFindings
Contested and acknowledgedCRF-8 (P4, default_agents_model_resource.go:140) - No test for Read "no models exist" path
Round logRound 1Panel. 0 P0-P2, 3 P3, 1 P4, 3 Nit. 1 dropped (comment style). Reviewed against bc80b0c..ddf528f. Round 2Churn guard: 6 addressed, 1 accepted (CRF-8, P4, resolved thread). Overrode BLOCKED to PROCEED (explicit thread resolution on P4). Reviewed against e61034a..50b203a. Round 3Churn guard: PROCEED (CRF-9 addressed). Reviewed against e61034a..57df609. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean pointer-resource implementation. The design correctly solves the cardinality mismatch that #371 identified: a global singleton modeled as a per-member boolean breaks Terraform's per-resource state model when the server mutates siblings out-of-band. The pointer resource eliminates that class of failure by construction.
Strengths: server-side verification in tests (checkServerDefaultMatchesResource queries the API directly), drift detection with out-of-band SDK mutation, no-op delete with CheckDestroy proving the server's default is untouched, steady-state re-plan proving no perpetual diff, and the integration test proving the pointer overrides Coder's automatic election end-to-end.
3 P3, 1 P4, 3 Nit. No blockers.
One observation not filed as a finding: Gon audited all 22 comments and found 16 restate identifiers, signatures, or framework behavior before reaching the useful information. The comments are accurate, and other reviewers found the documentation clear, so this is noted as a style observation rather than a defect. The minimum-draft rewrites in the Gon report are available if the author wants to tighten the prose.
"Boring, correct code." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
ethanndickson
force-pushed
the
ethan/integration-testcontainers
branch
from
bc80b0c to
e61034a
Compare
ethanndickson
force-pushed
the
ethan/default-agents-model-resource
branch
from
a123fc9 to
50b203a
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All six R1 fixes verified: redundant assertion removed, deferral test added and passing, warning includes config count, import comment corrected, dead UseStateForUnknown removed, experimental warning aligned. Each fix addresses its root cause without introducing new issues. CRF-8 (P4, accepted) remains unchanged.
One new finding: the CRF-4 fix corrected the Go source comment but missed the sibling text in import.sh, which feeds the generated docs.
0 P0-P2, 1 P3, 0 P4, 0 Nit.
"Every test in this suite proves behavior, not coverage." (Bisky)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All findings resolved across 3 rounds. 9 total findings: 7 fixed by the author, 1 accepted (CRF-8 P4, defensive edge-case test coverage), 1 dropped by orchestrator (CRF-6, comment style).
The CRF-9 fix (import.sh rewrite, 57df609) completes the import-documentation alignment. Generated docs match. Netero and panel (Mafuuu, Leorio, Kite) found no new issues.
Clean pointer-resource implementation with thorough test coverage. Ship it.
🤖 This review was automatically generated with Coder Agents.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Nice work. Just some non-blocking nits about the comments. My general assumption is that when this passes the integration tests it's good to go, although a manual smoke-test is probably not a bad idea.
Merge activity
|
ethanndickson
force-pushed
the
ethan/integration-testcontainers
branch
from
e61034a to
7c1cbed
Compare
ethanndickson
force-pushed
the
ethan/default-agents-model-resource
branch
from
0771788 to
cb3a4e2
Compare
ethanndickson
changed the base branch from
ethan/integration-testcontainers
to
graphite-base/378
ethanndickson
force-pushed
the
ethan/default-agents-model-resource
branch
from
cb3a4e2 to
16e854f
Compare
2 participants
Relates to CODAGT-607
Summary
Adds
coderd_default_agents_model, a singleton pointer resource that selects whichcoderd_agents_modelis the deployment-wide default chat model for Coder Agents:This completes the redesign started in #371.
is_defaultwas removed fromcoderd_agents_modelthere because Coder enforces exactly one default via a partial-unique index and mutates the flag out-of-band; a per-memberOptional + Computedboolean either plans as perpetual(known after apply)noise or, if pinned, trips "Provider produced inconsistent result after apply" when a sibling is set default in the same apply and demotes this model after the plan is frozen. Moving the choice to its own resource removes both failure modes by construction: there's no per-member computed flag to go unknown, and nothing for a sibling to mutate underneath it.Why a separate pointer resource
This is the established Terraform pattern for "exactly one of N is the server-enforced default" — model the pointer as its own resource, never a per-member flag. Three mainstream providers do exactly this:
aws_ec2_default_credit_specificationaws_ssm_default_patch_baselinegithub_branch_defaultThe closest analog is
aws_ec2_default_credit_specification: same framework stack, and almost exactly our shape — a chosen value plus a scope, Create/Update both call the same "set default" API, and Delete is a deliberate no-op because AWS always keeps some default per scope and you can't reach "zero defaults." That's precisely Coder's constraint (the partial-unique index plusensureDefaultChatModelConfigforce-promotion). HashiCorp'sresourcepackage docs also explicitly sanction the create-only / no-meaningful-delete shape for one-time operations that need tracking, so a singleton pointer is a documented, intended use.The counter-precedent is instructive:
aws_launch_template.default_versionkeeps anOptional + Computedflag on the member and tolerates the churn — but only because launch-template versions are not a mutually-exclusive singleton across siblings, so it never hits the "sibling demoted out-of-band during the same apply" inconsistency. That structural difference is exactly why they can keep the per-member flag and we can't.Behavior
UpdateChatModelConfig(model_id, { is_default: true }). The server's update handler is a read-modify-write merge that only overrides fields the request supplies, so a body of just{"is_default": true}preserves everything else, and the demote-of-previous + promote-of-new happens atomically in one transaction. This is an in-place update (likeaws_ec2_default_credit_specification), not a replace.model_id; if no models exist it removes the resource from state.coderd_agents_modelUUID and passes it through tomodel_id; Read then reconciles to the server's actual default, so a stale ID self-corrects on the next plan (mirrorsaws_ebs_default_kms_key, which imports by the value it points at).idis a computed constant (default) since the default is a global singleton with no scope key.Notes
The chat-model endpoints are experimental (
/api/experimental), so the resource is marked experimental via a warning callout in its docs, matchingcoderd_agents_model. Ships with generated docs/examples, unit + acceptance tests, and an integration-test resource that points the default at a non-first model to prove it overrides the server's automatic election end-to-end.