chore: add agents_allowed to templates - #27284
Conversation
ethanndickson
changed the title
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 11 findings (1 P1, 2 P3, 6 Nit, 2 Note), APPROVE. Review Finding inventoryFinding inventory - PR 27284Findings
Contested and acknowledgedCRF-3 (Note, queries/templates.sql:80) - filter has no production caller
CRF-5 (P3, 000544_...up.sql:1) - DEFAULT true inverts restrictive-allowlist policy
Round logRound 1Netero first pass + 17-reviewer panel. Base 21d0824..head e87c600. Effective +341/-1, 8 files, 77% test density. Law skipped (341 < 1000). Round 2Churn guard: PROCEED (5 addressed, 2 acknowledged, 0 silent). Author pushed fix commit 668c2e9 addressing CRF-2/4/6/7/8; CRF-3 and CRF-5 accepted as intentional by the human maintainer. Codex also raised the CRF-5 concern; same author reply. Effective +402/-1, 8 files, 78.4% test density. Law skipped (402 < 1000). Panel re-review to verify fixes (esp. CRF-4 EXCEPTION/fail-open block + new tests). Reviewed against 21d0824..668c2e9. Round 3Churn guard: PROCEED (4 addressed, 0 silent). Author fixed CRF-9 (retitled PR to 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: e87c6004b3
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a clean, tightly scoped slice: it lands the templates.agents_allowed column, backfills it faithfully from the deployment-wide allowlist, recreates template_with_names, adds a three-state nullable filter, and updates the audit table and generated docs in the same PR. Test density is 77% and the two new tests were run green against real Postgres by several reviewers. Backfill semantics were verified end to end against the runtime allowlist contract (isTemplateAllowed / chatTemplateAllowlist) and match exactly, and the authorization boundary is preserved: the filter is ANDed inside GetAuthorizedTemplates and cannot broaden access.
Severity count: 2 P3, 1 Note, 4 Nits. No P0/P1, so this is a comment, not a change request.
Both P3s are latent at this HEAD (nothing enforces the column yet) but are baked in here and cheapest to decide now:
- CRF-4: the migration trusts the stored allowlist shape; a malformed value aborts the upgrade with a cryptic Postgres error, diverging from the runtime's tolerate-and-allow-all behavior.
- CRF-5:
DEFAULT trueinverts the deny-by-default posture of a restrictive allowlist for templates created after the upstack cutover. This needs a human product decision, not an agent's acceptance: confirm default-allow for new templates is intended, or make the default deployment-controlled.
Process items (not blocking, but worth handling before merge):
- CI is red: 5 checks are failing and none are triaged on the PR. The PR's own new tests pass locally, so the added code is not the obvious culprit, but a red pipeline needs to be identified and either fixed or shown unrelated before merge.
- The single commit is typed
feat(coderd/database)while the PR title ischore(coderd/database). Pick one deliberately; the type drives changelog classification. - The
coderd/databasescope does not contain every changed file (enterprise/audit/table.go,docs/admin/security/audit-logs.md). Broaden or omit the scope per AGENTS.md.
Two sequencing notes worth tracking outside this PR: the backfilled agents_allowed is a point-in-time snapshot, so any admin edit to the old allowlist between this migration and the cutover is silently lost; and the backfill has no WHERE deleted = false, so soft-deleted templates also get agents_allowed = false under a restrictive list (no runtime effect today).
Bisky on the tests: "this test suite is lovely. Table-driven migration backfill, a filter matrix, story-free Go all the way down. And this time the stones are real. I ran both."
🤖 This review was automatically generated with Coder Agents.
ethanndickson
changed the title
ethanndickson
changed the title
|
@codex review |
|
/coder-agents-review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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.
Round 2. The fix commit 668c2e9 is clean work. Every R1 code finding is resolved and the panel verified each against the current code, several on a live Postgres:
- CRF-4 (fail-open) is fixed at the root: parsing and shape checks now run in a narrow inner
BEGIN ... EXCEPTION WHEN others THEN RAISE WARNING ...; RETURNblock, theUPDATEsits outside it, so corrupt legacy data leaves every template at theDEFAULT truewhile a genuine DB error on the write still aborts. Hisoka confirmed the only I/O left inside the swallow zone is none, so the scoping is tight, not incidentally tight. New subtests assert real per-template outcomes for invalid JSON, JSON object/scalar, invalid UUID, null element, and mixed valid/invalid, not just "no error". - CRF-8/CRF-2/CRF-6/CRF-7 verified fixed (parenthesized RHS + policy comment; no future-migration reference;
coderdtest.FakeAuthorizer.Preparereused; explicitBool: false). - CRF-5 and CRF-3 match the accepted product decisions; a human maintainer owns the default-allow posture and the DB-only stack boundary.
One blocker, and it is the R1 process note come true rather than new work:
- CRF-9 (P1): CI is red on the
titlecheck because the PR-title scopecoderd/databasedoes not containenterprise/audit/table.goordocs/admin/security/audit-logs.md. Both are correctly included in the diff (the audit registration and generated docs are required for the new auditable column), so the title must widen, not the diff shrink. This is a one-line fix (drop the scope, e.g.chore: add agents_allowed to templates, or broaden it) and it is what turns CI green. Note this corrects the R1 body note: the failure is the scope, not thefeatvschoretype; the title job acceptschore.
Requesting changes only to clear the red check. Two Nits and one Note below are optional.
Non-blocking notes for the record:
- Commit type
feat(head commit) vs PR titlechore: cosmetic, does not fail CI, and squash-merge uses the PR title. Pick one deliberately. - The fix commit subject
address agents_allowed review feedbacknames that treatment happened, not what changed; prefer something likefail open on corrupt agents_template_allowlistif intermediate commits survive. - Sequencing (outside this PR): the backfill is a point-in-time snapshot while
site_configsstays authoritative until the #27285 cutover, so an allowlist edit made between the two deploys is dropped unless #27285 re-backfills (Pariston). And corrupt legacy data now fails open permanently: after the cutover removes the legacy storage, a malformed value at upgrade time bakes in all-allowed with only a migration WARNING to show for it (Hisoka). Both follow from the accepted CRF-4/CRF-5 decisions; flagging so they are understood as fail-open-and-forget.
Hisoka, on the fail-open block: "I came looking for a fight in the fail-open block. It held."
🤖 This review was automatically generated with Coder Agents.
ethanndickson
changed the title
668c2e9 to
890a45a
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3. All prior findings are resolved or accepted, and the fixes hold. Netero plus an 11-reviewer panel verified this round; several ran TestMigration000552TemplateAgentsAllowedBackfill green against live Postgres.
Verified:
- CRF-9 (P1): the red
titlecheck is cleared. The PR was retitled tochore: add agents_allowed to templates(scope dropped), so the audit-table and docs files no longer fall outside a scoped path. CI is no longer red. - CRF-10/11/12: inner RAISE messages now name only the shape while the outer WARNING keeps the config-key context; the test case is renamed
empty string; and the down migration plus the post-migration delete run int.Cleanup(LIFO order verified) so a failed subtest no longer cascades duplicate-column errors. - Rebase and renumber (000544 -> 000552): consistent across every artifact. 000552 is the next free slot with no collision,
migrationVersion = 552, anddump.sql,models.go,modelqueries.go,queries.sql.go, the audit table, and the generated docs all agree.go build ./coderd/database/...is clean and no stray000544reference survives. - CRF-4 fail-open and CRF-5 default-true remain closed by maintainer acceptance; the code still matches those decisions after the rebase.
Pariston flagged that the all-stale-allowlist full-block case is untested; Bisky reconciled it as the same id = ANY(parsed_ids) false branch already pinned by templateIDs[1], a structural sibling rather than a gap. Not a finding.
Two metadata Nits remain, neither blocking:
- The PR description still calls this "Migration
000544" after the renumber to000552.grep 000544returns nothing in the tree, so the cited number does not exist; a squash-merge would bake it into the permanent commit message. One-token fix in the body. (Mafu-san rated this P3 for the squash-history permanence; recorded as a Nit here as a documentary fix in the same class as the earlier down-comment reference.) - The sole commit subject
feat(coderd/database): add agents_allowed to templatesstill carries the scope defect CRF-9 just fixed on the PR title (enterprise/audit/table.goanddocs/are outsidecoderd/database) and thefeatvschoretype mismatch. This is inert if the repo squash-merges (GitHub uses the PR title), so it only matters if intermediate commits survive; align it if so.
Dismissing the round 2 change request: the P1 blocker is fixed. Hisoka, after re-running the migration on live Postgres: "Boring survives. Silence is the rating."
🤖 This review was automatically generated with Coder Agents.
751984e to
9ca2c7e
Compare
aa6d1c2 to
ba41499
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba414996f6
ℹ️ 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".
ba41499 to
2946853
Compare
2946853 to
376f9a0
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.
2 participants
Relates to CODAGT-713
This adds
templates.agents_allowedas a default-true, auditable template attribute, along with nullable database filtering. Migration000562translates the effective legacyagents_template_allowliststate for existing templates: a valid nonempty list allows matching templates and blocks the rest, missing or empty values leave templates allowed, whilst corrupt values fail closed by blocking all existing templates. As per the linear issue, new templates deliberately default to allowed under the per-template model.This is the database-only first PR in the stack. #27285 makes the field authoritative in the API and chatd whilst temporarily retaining the compatibility routes needed by the shipped frontend. Later PRs migrate the UI, remove the legacy storage, routes, SDK types, and utility, then add CLI flags.