chore: remove legacy chat template allowlist - #27515
Conversation
7590a7a to
f90d237
Compare
ca6e762 to
35387b5
Compare
f90d237 to
d1cdd80
Compare
35387b5 to
616f118
Compare
0e1a0b6 to
8acb538
Compare
4dcfdf6 to
33c31a7
Compare
8acb538 to
0e64d11
Compare
|
@codex review |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 12 findings (1 P1, 8 P3, 1 P4, 1 Nit, 1 Note), REQUEST_CHANGES. Review Finding inventoryFinding inventory, PR #27515Findings
Contested and acknowledgedNone. Round logRound 1Netero first pass, then a 20-reviewer panel (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Gon, Leorio, Knuckle, Kurapika, Kite, Ryosuke, Knov, Ging-Go, Ging-TS, Nami, Chopper, Melody, plus wildcards Zoro and Meruem). Law not spawned: effective additions are 4. Netero reported 2 P2 (docs), 2 P3, 1 Nit, 1 Note. Orchestrator verification of Netero's two P2s (docs drift):
Convergence, CRF-4 raised to P1 as CRF-6. Seven reviewers independently reached the same location from different triggers:
Consequence chain: an access control on agent workspace creation fails open, silently (missing row and unconfigured are the same value in the old reader), across three independent triggers, with no recovery, and the DELETE buys nothing at head because no code reads the row after this diff. Orchestrator raised to P1 and merged CRF-4 into CRF-6, since one fix (drop 000563) closes all three. Independent orchestrator verification of the claim driving the P1: Downgrade gate on Mafu-san's P1 for red CI (CRF-8):
No findings from Bisky, Ryosuke, Chopper, Melody, Nami, Ging-Go, or Ging-TS. All seven verified the deletion's completeness independently and agreed: no dangling reference, no orphaned test helper, no lost coverage, no swagger or apidoc regeneration owed. 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: 0e64d11799
ℹ️ 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: re: P1 Badge Preserve the legacy allowlist for rollback This is intentionally destructive, same as Restoring would be worse in the common case: a deployment that never configured an allowlist has every template at
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e64d11799
ℹ️ 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 re: Preserve the legacy allowlist for rollback This is intentionally destructive, same as 000387 and 000555. We don't support rollbacks, and the only thing lost is a list of template IDs an admin can set again from the AI Settings page, so I'd rather accept the loss than restore it. Restoring would be worse in the common case: a deployment that never configured an allowlist has every template at agents_allowed = true, so we'd write out exactly the templates that existed at rollback time and old chatd would deny anything created after that. Fail-closed for everyone, to avoid a fail-open for the few deployments that set a list. 000562 already drops agents_allowed on the way down, so the rollback loses data regardless of what 000563 does. re: Retain the allowlist through mixed-version rollouts Deliberately not deferring this. |
|
Codex Review: Didn't find any major issues. Bravo. 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Documentation CheckThis PR removes the legacy deployment-wide chat template allowlist (the The PR description defers the platform controls docs to the rest of the stack. This PR's diff contains no docs changes, so the items below remain open. Updates Needed
Automated review via Coder Agents |
ethanndickson
changed the title
0e64d11 to
f5a8248
Compare
|
@codex review: re: P1 Retain the allowlist through mixed-version rollouts Keeping the DELETE. The window is real but it's bounded by the rollout, and The rollback half of this doesn't hold up either. We also don't defer destructive changes across releases anywhere in this repo. I've made the down migration comment honest about what it deleted, since the old wording described 000562's backfill rather than this migration. |
86acdb5 to
9847266
Compare
2932f73 to
c18baff
Compare
a5561c0 to
e7f74e3
Compare
73e9ab6 to
e1df7ab
Compare
6d831b3 to
c16f754
Compare
6cd0526 to
ccd94e6
Compare
c16f754 to
ccc4841
Compare
Relates to CODAGT-713 Depends on #27284 This makes the per-template `agents_allowed` field authoritative in the API and chatd. It adds optional create and metadata update fields with the intended default and omission semantics, supports `agents-allowed:` template search, includes the value in telemetry, and makes `list_templates`, `read_template`, and `create_workspace` read the template row directly. Existing-workspace retries remain idempotent, and blocked same-organisation templates return an actionable message. The experimental `/template-allowlist` routes remain temporarily because the shipped AI Settings page still calls them, but they no longer control chatd enforcement. #27514 moves that page to per-template metadata, #27515 removes the legacy storage, routes, SDK types, and utility, #27517 adds the CLI flags, and #27518 updates the platform controls documentation for the per-template model, directly addressing CRF-5 and CRF-6. The stack is intended to merge as a unit.
Relates to CODAGT-713 Depends on #27285 This replaces the deployment-wide template allowlist UI with per-template Coder Agents controls. Each template's General Settings page exposes `agents_allowed` to organisation and template administrators through the existing template-update permission, whilst AI Settings > Templates gives deployment administrators a bulk management page. The bulk page requires both deployment-config and template-update permissions, reuses the same URL-backed server-side filter as `/templates`, tracks pending updates independently for each template, and surfaces failures through template-specific toasts. The copy refers to new workspace creation because disabling a template does not revoke an existing Coder Agent workspace. This completes the frontend migration away from the experimental `/template-allowlist` API. #27515 removes the now-unused legacy storage, routes, SDK methods, generated types, and JSON utility. Later PRs add CLI flags and update the platform controls documentation. The stack is intended to merge as a unit.
ccc4841 to
a23ce3a
Compare
2 participants
Relates to CODAGT-713
Depends on #27514
Removes the legacy deployment-wide allowlist now that the API and frontend use per-template
agents_allowed: the experimental/template-allowlistroutes, SDK methods and generated types, site config queries, frontend bindings, and the now-unusedxjsonutility.Migration
000563deletes the obsoleteagents_template_allowlistvalue. It's irreversible for deployments that configured an allowlist, which I think is fine, since000562already dropsagents_allowedon the way down, and this release ships000548and000555with the same property.Two side effects of the model change worth writing down, both from #27514 rather than here. The value used to need
ActionReadonResourceDeploymentConfigto read and deployment config update to write.AgentsAllowedis now a plain field on the template response, readable by anyone who can read the template, and it's set with a template update, so org admins manage it themselves. That's the delegation we wanted, and it's tracked in the audit log.The rest of the stack adds
--agents-allowedto the CLI and updates the platform controls docs.