feat(coderd_template): expose agents_allowed - #414
Conversation
ethanndickson
changed the title
b98c08b to
4293f72
Compare
![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: 4293f72bf7
ℹ️ 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".
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 4 | Last posted: Round 4, 23 findings (4 P2, 3 P3, 14 Nit, 2 Note), COMMENT. Review Finding inventoryFinding inventory: PR #414Findings
Contested and acknowledgedCRF-1 (P2, template_resource.go:1463) - Version gap for
|
| Reviewer | Focus |
|---|---|
| Bisky | tests |
| Chopper | ops/errors |
| Churn-guard | change verification |
| Ging | language modernization |
| Gon | naming |
| Hisoka | edge cases |
| Killua | perf |
| Kite | change integrity |
| Knov | contracts |
| Knuckle | SQL |
| Komugi | flake/determinism |
| Kurapika | security |
| Law | decomposition |
| Leorio | docs |
| Luffy | product |
| Mafu-san | process |
| Mafuuu | contracts |
| Melody | dispatch/pairing |
| Meruem | structural |
| Nami | frontend |
| Netero | mechanical checks |
| Pariston | premise testing |
| Pen-botter | product gaps |
| Razor | verification |
| Robin | duplication |
| Ryosuke | Go arch |
| Takumi | concurrency |
| Zoro | shape |
🤖 Managed by Coder Agents.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two commits, one feature. The agents_allowed plumbing is clean end-to-end and Netero's mechanical sweep found nothing. boolPtrOrNil earns its keep, framework ValueBoolPointer() returns &false for Unknown and the helper intercepts at the pointer boundary, four table-tested cases, right primitive at the right layer. The reconcileUpdateResponse extraction closes the pre-existing asymmetry where Update didn't reconcile UseClassicParameterFlow from the server response, and TestAccTemplateResourceAgentsAllowed round-trips omitted, false, true, and unmanaged. EqualTemplateMetadata is extended so an agents_allowed-only change triggers PATCH.
Two P2 findings worth naming before merge:
-
The version-gap failure mode for
agents_allowedon a <v2.37 Coder server.codersdk.Template.AgentsAllowedis a plainboolwith noomitempty, so an older server that omits the field decodes tofalse;readResponse/reconcileUpdateResponsethen pinfalseinto state. Two consequences:agents_allowed = truein config hard-fails at apply time with "Provider produced inconsistent result after apply", and the omitted case creates a silent-drift cascade after the operator upgrades Coder (saved-plan CI workflows that skip refresh will PATCHagents_allowed: falseon the next update to any other attribute, disabling agents on every managed template with no plan-visible diff). The docs warnRequires a Coder deployment running v2.37.0 or later, but nothing enforces it, and the PR body claim of "keeps older Coder deployments compatible" only covers the omitted case, not the pinned-state cascade. The failure class matchesMaxPortShareLevel,CORSBehavior, andUseClassicParameterFlowon their respective minimums, but this PR could gate atConfigure()time via the existingBuildInfocall. -
The class-of-bug the PR just diagnosed still ships in the sibling.
use_classic_parameter_flowhas the identical schema shape asagents_allowed(Optional+Computed+UseStateForUnknown, noDefault:) and still usesr.UseClassicParameterFlow.ValueBoolPointer()at bothtoCreateRequest(line 1557) andtoUpdateRequest(line 1511). It is masked today only because the current server default coincidentally matchesfalse; the SDK's own comment warns the default may flip. AGENTS.md rule: "one correction implies every instance." Routing both call sites throughboolPtrOrNilwould make the compensating dance at 686-701 strictly narrower.
Process observation the diff hides: the fix(coderd_template): reconcile metadata after update commit is a strict behavior change for UseClassicParameterFlow on Update, not just an extraction. Pre-PR, Update only wrote back MaxPortShareLevel and CORSBehavior; post-PR, UseClassicParameterFlow is also reconciled from the server response. Neither the PR description nor the fix commit body mention this. On v2.37+ Coder the change is invisible; against a server that previously ignored the field it converts silent drift into an "inconsistent result after apply" error at Update time. Strict improvement, but a future bisect for a UseClassicParameterFlow regression will land on feat: expose template agents_allowed (#414) with nothing in the log explaining why. One sentence in the PR body or the fix commit body naming the second field would close the gap.
Zoro's line on the sibling put it well: "That is a latent, not absent, bug: a coincidence of defaults, not a design invariant, is what saves you."
Counts: 2 P2, 3 P3, 7 Nit, 1 Note.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@codex review |
|
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". |
There was a problem hiding this comment.
R2 delivered a proportional, focused commit. 4acf837 addressed 10 of 13 R1 findings at the root, not just at the reported call sites: boolPtrOrNil was extended to the sibling UseClassicParameterFlow in both toCreateRequest and toUpdateRequest, the reconcile helper was renamed to reconcileVersionedMetadata and takes a pointer receiver matching readResponse, the templateAgentsAllowedMinVersion const now supplies all four call sites, and the reconciliation unit test seeds distinguishing values so any pairwise swap in the mapping fails. No scope drift, no drive-by edits.
Panel decisions on the three findings still open at the start of R2:
-
CRF-1 (P2) re-raised. The author acknowledged the pre-v2.37 plain-boolean collapse and defended it as "matches established behavior for similar versioned template attributes." The panel voted 2 P2 re-raises (Mafuuu, Pariston) with new differentiating evidence, plus 4 concurring reviewers (Hisoka, Meruem, Kite, Mafu-san) that the "consistent with siblings" defense does not cover
agents_allowed's specific silent-drift mode. Pariston compared the four versioned attributes:MaxPortShareLevelfails loudly (invalid enum),CORSBehavioris structurally immune (stringValueOrNullmaps the absent field to null),UseClassicParameterFlowis a functional no-op (phantom-false matches server default), andAgentsAllowedis the first attribute in the class whose collapse produces a valid-but-wrong value that flips a feature default off with no diagnostic. Mafuuu named the shape sharply: the phantom-false is the opposite of the post-upgrade server default. Human decision needed. No ticket linked. File a ticket for a provider-wide version gate atConfigure()(mirroring the existingEntitlements()cache) with a plan-time validator that rejectsagents_allowedon pre-v2.37 deployments, or explicitly accept the gap with a maintainer sign-off. "Matches the pattern" is not a resolution once the differentiating consequence is on the table. -
CRF-10 (Nit) closed. Panel accepted the author's narrow-scope defense 5/6: the TODO explains the specific case of
MaxPortShareLevel; the structural pressure sits on CRF-13 (the mapping asymmetry itself), not on this comment. The rename toreconcileVersionedMetadatais now the readable rule. -
CRF-13 (Note) re-raised. Meruem's new evidence: substituting
data.reconcileVersionedMetadata(&template)forRead's lines 743-745 is a byte-for-byte equivalent replacement.Readhas no compat logic; the R2 rename delivered a helper whose scope is one caller when a second was trivially available. The author's Create-side compat argument (planned values must survive until PATCH decisions finish) is valid for Create and does not carry Read. Mafuuu, Pariston, Mafu-san concur that ticketless "deferred to separate work" is a drop under the review contract, not a deferral. Human decision needed. No ticket linked. File a ticket for at minimum the Read-side collapse and the accompanyingMaxPortShareLevel deliberately omittedTODO cleanup, or accept the fragmentation with a maintainer sign-off.
New this round: CRF-15 (Netero + Mafu-san concur, request-side sibling coverage gap for AgentsAllowed), CRF-16 (Kite, stale TODO scope at line 913 whose deletion at a hypothetical v2.15+ minimum would silently regress state reconciliation for v2.37+ fields), CRF-17 (Gon + Leorio, boolPtrOrNil doc comment is imprecise and vague), CRF-18 (Pariston, TestAccTemplateResourceAgentsAllowed step 4 tests preservation of true when the server default is also true, so UseStateForUnknown's contribution is not observable; swapping the step to run after cfgFalse would fix this).
Process note the diff does not carry: the PR description still reads "Omitted values use the server response, which keeps older Coder deployments compatible." Given the R2 acknowledgment on CRF-1, that sentence overclaims relative to what ships. The pre-v2.37 behavior for omitted config pins false into state and PATCHes false back on the next Update, which is silent-drift, not compatibility. A future maintainer reads the description, not the panel debate. Update the description or link the CRF-1 tracking ticket.
Mafuuu's line captured the CRF-1 disagreement: "the phantom-false is the opposite of the post-upgrade server default."
Counts: 2 P2 re-raised, 1 Nit closed, 1 Note re-raised, 2 new Nits, 1 new Note, and 10 R1 fixes verified.
internal/provider/template_resource.go:913
Nit [CRF-16] TODO(ethanndickson): Remove this once the provider requires a Coder deployment running v2.15.0 or later now scope-lies about the block below it. (Kite)
Kite: "The client.Template(ctx, templateID) GET that follows the TODO existed because UpdateTemplateMeta's response on pre-v2.15 Coder omitted MaxPortShareLevel. After this PR the GET also feeds AgentsAllowed (added in v2.37) and the newly-routed UseClassicParameterFlow through reconcileVersionedMetadata. A maintainer who follows the TODO's instruction at a hypothetical v2.15+ minimum would delete the GET (and the reconcile), silently regressing state reconciliation for v2.37+ fields."
Update the TODO to name the actual condition ("once the update response is authoritative for MaxPortShareLevel/CORSBehavior/UseClassicParameterFlow/AgentsAllowed"), or drop the version reference and describe what the block does.
🤖
🤖 This review was automatically generated with Coder Agents.
ethanndickson
changed the title
|
CRF-16: No change. The maintainer requested that the existing v2.15 compatibility TODO remain. This PR does not introduce a new minimum Coder version for |
|
/coder-agents-review |
There was a problem hiding this comment.
R3 delivered a proportional test-side fix (af41226) and a substantive PR body rewrite. Three findings closed: CRF-15 (request-side coverage for AgentsAllowed), CRF-17 (partial: boolPtrOrNil comment now names the pointer), CRF-19 (PR body narrows the compatibility claim to explicit-use only). CRF-18 is also closed by the panel: Pariston withdrew after tracing the framework contract; Razor verified that removing UseStateForUnknown from the Optional+Computed attribute leaves the plan value Unknown against a known state, so PlanOnly: true catches modifier removal at step 4 regardless of which value happens to sit in state. Author's plan-shape defense holds.
Panel re-raises four findings this round.
-
CRF-1 (P2) re-raised 7-0. Pariston brought new AGENTS.md evidence: "If an invariant is decidable from config + prior state, enforce it in a validator,
ValidateConfig, or plan modifier instead of returning an error fromCreate()/Update(), a failed apply is strictly worse UX than a failed plan." The scaffolding already exists:CoderdProviderDatacachesEntitlements()atConfigure()(provider.go:44,211); a two-line extension addsBuildInfo()on the same call and exposes aRequireServerVersion(min)helper. Kite, Meruem, Razor, and Hisoka independently traced the silent-drift cascade against the current code: on <v2.37 create-with-omitted, state pinsfalse; on the first refresh-skipping Update after upgrade,boolPtrOrNil(BoolValue(false)) = &falsePATCHesagents_allowed: false, silently disabling agents on every managed template. The R3 body rewrite narrows the compatibility claim to explicit-use only; it does not describe or accept the omitted-config silent-drift cascade. Mafu-san also flagged that the R3 "Maintainer decision:" prefix is delivered by an agent-generated reply ("Response generated by Coder Agents") from the maintainer's account without a distinct human-authored sanction, so the panel-required condition is not visibly satisfied. Human decision needed. File a ticket for theBuildInfocache + plan-time validator (or land it in this PR), or add a visible human-authored acceptance to the PR body that names the omitted-config silent-drift scenario. -
CRF-13 (Note) re-raised 3-2. Panel split. Kite and Razor close accepting the author's Create-side compat argument. Meruem, Pariston, and Mafuuu re-raise on the Read-side: substituting
data.reconcileVersionedMetadata(&template)forRead's lines 743-745 is byte-for-byte equivalent; the R2 rename toreconcileVersionedMetadataexplicitly narrowed the helper's scope, and the trivial second caller was one line away in the same PR. Under "reviewer disagreement is doubt, higher wins," re-raise stands. Hisoka: "weak accept, ticket wanted." Human decision needed. File a ticket for the Read-side collapse (or land it in this PR), or accept the fragmentation with a visible human-authored sanction. -
CRF-16 (Nit) re-raised 5-1. Panel notes the TODO says "Remove this once the provider requires ... v2.15.0 or later," but the
client.TemplateGET at line 915 now feedsreconcileVersionedMetadata, which reconcilesMaxPortShareLevel,CORSBehavior,UseClassicParameterFlow, andAgentsAllowed. Razor traced thattemplateResphas only one call site (line 920, the reconcile), so the TODO's stated removal criterion no longer describes the block below it. Author's R3 defense concedes the scope shift while declining to update the text. Kite (original raiser) closes from their side on the argument that deleting lines 915-919 would break line 920's compilation, forcing engagement. The panel majority weighs the misleading text over the compile safety net. Fix: update the TODO to name the actual criterion (e.g., "once the update response is authoritative for all versioned template metadata") or drop the version reference. -
CRF-17 (Nit) re-raised. The R3 rewrite fixed part (a) ("a pointer to false" now matches
stringPtrOrNil's&""convention) but introduced a factually wrong claim: "sends a deferred value in the request." Hisoka, Mafu-san, Leorio, and Razor independently verified that in this framework, "deferred" refers toresource.NewDeferredResponse, an operation postponed because inputs are unknown; the mechanism here is the opposite (the unknown is eagerly emitted asfalseand persisted). Suggested wording that mirrorsstringPtrOrNiland names the mechanism:// ValueBoolPointer returns &false for unknown, which can accidentally send false.
Six new findings this round.
- CRF-20 (Nit, Netero + panel):
TestTemplateResourceBoolRequests(added in R3 to close CRF-15) bindsUseClassicParameterFlow: tc.valueandAgentsAllowed: tc.valuein the same model; a swap of the twoboolPtrOrNil(...)assignments passes every subtest. Same class as CRF-8, applied to the request paths. Fix: distinct values in the model, or split into two tables. - CRF-21 (P2, Mafuuu): Data source sibling of CRF-1.
data.AgentsAllowed = types.BoolValue(template.AgentsAllowed)attemplate_data_source.go:342inherits the plain-bool decode pathology. Blast radius: users who explicitly referencedata.coderd_template.foo.agents_allowed(per the exampledata-source.tf) on pre-v2.37 propagatefalseinto cloned template config. - CRF-22 (Nit, Mafuuu): Data-source schema description at
template_data_source.go:190says "Requires a Coder deployment running v2.37.0 or later" without stating the pre-v2.37 decode behavior. A data source has no "explicit use" axis (it is always read), so the resource-side scope narrowing does not carry. - CRF-23 (P2, Leorio): The
agents_allowed = trueon pre-v2.37 apply-fail surfaces as "Provider produced inconsistent result after apply. This is a bug in the provider," without naming the cause or the fix. The siblingai_providerresource already solved this class:checkBedrockRoleARNDropped(ai_provider_resource.go:687) andcheckBedrockProtocolDropped(line 706) compare configured vs response and emit a targeted "server accepted the request but did not persist X" diagnostic. Mirror ascheckAgentsAllowedDropped(config, template, diags)called from Create and Update. - CRF-24 (Nit, Gon): Drive-by punctuation edits to two pre-existing test-step comments (
template_resource_test.go:344,:358) that are unrelated toagents_allowedand inconsistent with sibling comments in the same table. AGENTS.md: "Touch only what the task requires. No unrelated refactors, renames, or formatting churn." Revert. - CRF-25 (Nit, Leorio):
TestTemplateResourceReconcileVersionedMetadatasubtest name"values"says nothing about what it tests. Rename to describe the behavior (e.g.,"overwrites all fields with response") to match the sibling's scenario-naming shape.
Hisoka's line captured the R3 disposition on CRF-1: "The description rewrite narrates the scope; the code trap is intact."
Counts: 4 re-raised (2 need a ticket or human sanction), 1 closed, 6 new, 4 R2/R3 fixes verified.
internal/provider/template_resource.go:913
Nit [CRF-16] Panel re-raise (5-1) with new evidence. (Hisoka, Mafu-san, Mafuuu, Meruem, Razor re-raise; Kite closes from their own side; Pariston as-Nit)
Razor traced the GET's uses: templateResp (returned at line 915) is referenced only once, at line 920 (newState.reconcileVersionedMetadata(&templateResp)). The TODO's stated removal criterion (v2.15+ minimum) no longer describes the block below it, which now reconciles MaxPortShareLevel (v2.15+), CORSBehavior (v2.26+), UseClassicParameterFlow, and AgentsAllowed (v2.37+). The author's R3 defense concedes the scope shift ("the reconciliation now also uses its authoritative response for the versioned fields") while declining to update the text.
Kite (original raiser) closes on the argument that deleting lines 915-919 would break line 920's compilation, forcing engagement; the majority weighs the misleading TODO over the compile safety net because the maintainer following the TODO's letter could delete both blocks together and silently regress state reconciliation for the v2.26+/v2.37+ fields.
Meruem's concrete rewrite: // client.Template GET here reconciles versioned fields (max_port_share_level, cors_behavior, use_classic_parameter_flow, agents_allowed) that Update responses may not reflect on older servers. Remove only once every versioned field here is guaranteed authoritative in the update response. Naming the fields makes the deletion trap impossible to trigger.
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
R4 delivered a clean, proportional test-side commit (4c41a1a). Four findings verified fixed at their root: CRF-17 (boolPtrOrNil comment now &false, which can accidentally send false, matching stringPtrOrNil's shape), CRF-20 (TestTemplateResourceBoolRequests uses distinct values and per-field expected pointers across four subtests; swap of either request-side boolPtrOrNil(...) assignment fails a specific subtest), CRF-24 (two drive-by punctuation edits reverted byte-for-byte to base state), CRF-25 (subtest renamed from "values" to "overwrites all versioned fields"). Netero: no findings.
Six findings are re-raised with new R4 evidence.
- CRF-1 (P2) partial-acceptance evaluation. The R4 PR body now names the saved-plan/no-refresh omitted-config scenario the R3 panel asked for (
The PR author accepts this narrow compatibility edge). That covers one downstream consequence of the plain-bool decode. Two other consequences of the same premise remain unaccepted anywhere in the diff or PR body: explicitagents_allowed = trueon <v2.37 (CRF-23) and the data-source silent-falseon <v2.37 (CRF-21). The R4 reply carrying the body update is signedResponse generated by Coder Agents.The panel sees the body sentence as substantive (Kite: accept the saved-plan scope; Meruem, Knov, Pariston, Mafuuu: not sufficient because the scope covers only one of the three known failure modes). Under "no agent-accepted permanence," a human decision that names the two remaining scenarios is still required, either by extending the PR body sentence to name them, by linking a follow-up ticket per accepted scenario, or by landing the two-lineBuildInfocache + plan-time validator that closes the class at its source (CoderdProviderData.Configure()already cachesEntitlements()atprovider.go:44,211;BuildInfo()fits the same idiom). - CRF-13 (Note) re-raised with new R4 evidence.
AgentsAllowedis now written twice: once atreconcileVersionedMetadata(template_resource.go:1434) and again atreadResponse(template_resource.go:1465). The R2 rename explicitly narrowed the helper's scope; this PR then routedAgentsAllowedthrough both helpers under different membership rules. A fifth versioned attribute now has five possible homes (readResponse-only,reconcileVersionedMetadata-only, both,Read-inline, Create-compat-block). The R4 reply restates the R2 Create-side compat defense and does not engage this new double-write evidence. - CRF-16 (Nit) re-raised. The R3 5-1 panel re-raise cited Razor's trace that
templateResphas a single call site at line 920, and that following the TODO'sv2.15.0criterion would silently regress state reconciliation for v2.37+ fields. The R4 delta does not touch line 913, and no new author reply engages the R3 evidence. Fix cost: rewrite the TODO to name the actual criterion (e.g.,once the update response is authoritative for max_port_share_level, cors_behavior, use_classic_parameter_flow, agents_allowed). No behavior change. - CRF-21 (P2) re-raised 6-0. The R4 defense (
Reading agents_allowed from the data source is explicit use) conflates data-source read with attribute reference:Readruns on every data-source lookup regardless of whether the operator references.agents_alloweddownstream. On <v2.37 the write emitsfalsefor a template whose server default istrue, with no diagnostic. The R4 PR body acceptance is scoped to the resource-side saved-plan omitted-config edge; the data-source explicit-read scenario is not named and cannot be inherited by scope expansion in an agent-signed reply. Minimum-viable fix: gate the write behind aBuildInfoversion check (or upstreamcodersdk.Template.AgentsAllowedto*boolwithomitempty). - CRF-22 (Nit) re-raised 5-0. The R4 defense points at the PR body; users read the generated
docs/data-sources/template.md, not the PR body. The current schema description promises "Requires a Coder deployment running v2.37.0 or later" and does not state what the pre-v2.37 read returns. One-line append flows throughmake genat no schema cost:On Coder deployments before v2.37.0 the field is absent from the API response, and this data source therefore returns false. - CRF-23 (P2) re-raised 7-0. The sibling
ai_provider_resource.goalready solved this class withcheckBedrockRoleARNDropped(line 687) andcheckBedrockProtocolDropped(line 706): compare configured value to server response, emitAddAttributeErrornaming the version gap. The R4 defense (outside the accepted compatibility scope) is a scope restatement that does not engage the copy-shape prior art in the same repository. Prescription: addcheckAgentsAllowedDropped(config, template, diags), call from Create afterreadResponseat line 628 and from Update afterclient.Templateat line 915 beforereconcileVersionedMetadata. Ships independent of CRF-1; the two remedies compose.
Two new findings this round.
- CRF-26 (Nit, Gon).
TestTemplateResourceBoolRequestssubtest names encode field pairs positionally ("unknown and true","true and unknown"). The reader must open the table to map each value to a field. The file's own convention names the scenario (TestTemplateResourceReconcileVersionedMetadatauses"overwrites all versioned fields";agents_model_resource_test.gouses"whitespace and key order"). Rewrite to name both fields, e.g."classic unknown, agents true". - CRF-27 (Nit, Leorio).
reconcileVersionedMetadataships without a doc comment. The rule thatreadResponseandreconcileVersionedMetadatatogether cover the versioned fields (withAgentsAllowedwritten by both) is not readable off the code today. Three lines above the func name the pairing rule and close part of CRF-13's structural complaint at the cheapest layer.
Process observation. Every author reply on the six re-raised threads is signed Response generated by Coder Agents. The R3 panel flagged this attribution pattern; R4 dropped the Maintainer decision: prefix on one reply (CRF-1) and left the pattern intact on the other five. Mafu-san verified that no non-agent author comment exists on the PR conversation for R4 (comments-full.md). The R4 CRF-1 body sentence is the closest visible human-adjacent artifact; the CRF-16 defense claims "the maintainer requested" via an agent-signed issue comment (IC_kwDOMGJiIM8AAAABPZyPoA) with no visible source. What would resolve the class in this PR without more code: a comment or PR-body byline from @ethanndickson without the Response generated by Coder Agents. footer that names the four scenarios (omitted-config saved-plan drift, explicit-true apply-fail, data-source silent-false, generated-doc omission) and either accepts each or links a ticket per accepted item.
Mafu-san on the R4 attribution: "dropping the visible prefix while keeping the pattern is self-awareness without behavior change, which is worse than not knowing because it looks like learning."
Counts: 4 R3 findings verified fixed, 6 re-raised (2 P2 with panel decision on partial acceptance, 3 P2 re-raised on their own axis, 1 Note re-raised with new R4 evidence, 1 Nit re-raised, 1 Nit that already re-raised R3 and stayed silent R4), 2 new Nits.
internal/provider/template_resource.go:913
Nit [CRF-16] R3 panel re-raise (5-1, new Razor evidence) stayed silent in R4. (Hisoka, Pariston re-raise)
R4 delta does not touch line 913. No new author reply engages the R3 evidence: templateResp at line 915 has a single call site at line 920 (newState.reconcileVersionedMetadata(&templateResp)), which reconciles MaxPortShareLevel (v2.15+), CORSBehavior (v2.26+), UseClassicParameterFlow, and AgentsAllowed (v2.37+). A maintainer honoring the TODO at a hypothetical v2.15+ minimum deletes the block and silently regresses state reconciliation for the v2.26+/v2.37+ fields; the compile-safety net Kite cited (deleting the block breaks line 920) is one adjacent line to delete in the same edit and no test would catch that either.
The R3 issue-comment defense (IC_kwDOMGJiIM8AAAABPZyPoA, The maintainer requested that the existing v2.15 compatibility TODO remain) is agent-signed with no visible non-agent artifact. Under "no agent-accepted permanence" the claim of maintainer intent needs a distinct source.
Minimum-viable fix: update the TODO to name the actual removal criterion, e.g., TODO: remove once the update response is authoritative for max_port_share_level, cors_behavior, use_classic_parameter_flow, agents_allowed. No behavior change.
🤖
🤖 This review was automatically generated with Coder Agents.
eda33a0 to
8ece685
Compare
8ece685 to
2b73027
Compare
2b73027 to
ec39e20
Compare
Bumps `github.com/coder/coder/v2` from the previously pinned main commit
(`fbac602456c9`, 2026-07-28) to a newer `main` commit (`8a7e8d9d5b31`,
2026-08-19).
Resulting module version: `v2.34.0-rc.0.0.20260819135125-8a7e8d9d5b31`
## Breaking change
This SDK bump removes pricing fields from
`coderd_agents_model.model_config`. The following settings are no longer
supported and must be removed:
```hcl
model_config = jsonencode({
cost = {
input_price_per_million_tokens = "3"
output_price_per_million_tokens = "15"
}
})
```
The four legacy top-level pricing settings are also removed:
- `input_price_per_million_tokens`
- `output_price_per_million_tokens`
- `cache_read_price_per_million_tokens`
- `cache_write_price_per_million_tokens`
The provider validates `model_config` against its pinned SDK.
Configurations with these pricing settings will fail at plan time.
Remove these settings before you upgrade the provider. Coder now gets
cost data from AI Gateway.
The example and generated documentation remove `model_config.cost`.
## Compatibility
This SDK bump does not set a new minimum Coder version for the
`coderd_template` resource. The follow-up #414 adds the optional
`agents_allowed` attribute. Explicit use of that attribute requires
Coder v2.37.0 or later.
## Release context
This change requires a provider release that is marked as breaking. The
release notes must include the `model_config` pricing migration. The
breaking change applies to `coderd_agents_model`.
This is the prerequisite layer for #414 and also includes the exported
`codersdk.SanitizePromptText` used by #412.
Refs
https://linear.app/codercom/issue/CODAGT-716/expose-agents-allowed-in-the-coderd-terraform-provider
<details>
<summary>Stack plan</summary>
1. Update the Coder SDK and remove obsolete chat model cost
configuration.
2. Add `agents_allowed` to the `coderd_template` resource and data
source in #414.
3. Add the deployment-wide chat system prompt resource in #412.
</details>
_Opened by Coder Agents on behalf of @ethanndickson._
2 participants
Expose the per-template
agents_allowedflag through thecoderd_templateresource and data source. Terraform can now create, update, and read whether Coder Agents can use a template.The change does not set a new minimum Coder version for existing
coderd_templateconfigurations. If the configuration omitsagents_allowed, the provider does not send the field during create. Older Coder servers omit the field in responses, so the provider reads it asfalse, consistent with other versioned template attributes.The PR author accepts these compatibility limits:
agents_allowedconfiguration requires Coder v2.37.0 or later. An older server will ignore the unsupported field and cause an inconsistent-result apply error.falseforagents_allowedon older servers because the API response omits the field.falseinto an unrelated template update after the upgrade. This only disables Coder Agent access for that template. It does not affect ordinary workspaces or damage the template. Users can restore access by settingagents_allowed = truein a fresh plan.Users must create a fresh Terraform plan with refresh enabled after upgrading Coder to v2.37.0. They must not apply a saved pre-upgrade plan after the upgrade.
Acceptance coverage runs only when the Coder image reports v2.37.0 or later.
Depends on #413.
Closes https://linear.app/codercom/issue/CODAGT-716/expose-agents-allowed-in-the-coderd-terraform-provider
Stack plan
Update the Coder SDK and remove obsolete chat model cost configuration in chore!: bump coder/coder SDK and remove model cost settings #413.
Add
agents_allowedto the resource and data source, including generated documentation and version-gated acceptance coverage.Add the deployment-wide chat system prompt resource in feat(coderd_chat_system_prompt): manage the deployment-wide chat system prompt #412.
This PR was generated by Coder Agents.