feat: add per-template Coder Agents access control - #27285
Conversation
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 7 | Last posted: Round 7, 15 findings (3 P2, 2 P3, 6 Nit, 4 Note), APPROVE. Review Finding inventoryFinding inventory - PR #27285Findings
Contested and acknowledgedCRF-8 (P3, coderd/exp_chats.go:6179) - retained allowlist write surface is a silent enforcement no-op
CRF-15 (Note, coderd/x/chatd/chattool/readtemplate.go:65) - no shared chokepoint for the AgentsAllowed gate
Round logRound 1Netero-only first pass (pre-panel gate). Effective LOC +214 (below Law threshold, Law not run). Netero found 1 P2 + 3 Notes. Per the Netero decision gate, a P2 in a pre-panel round skips the panel and posts a first-pass-only COMMENT review; the panel reviews after the P2 is addressed. Reviewed against e87c600..22cf836. Round 2Churn guard PROCEED: all 4 R1 findings classified addressed in db7413e (author claims; verified on re-review). CRF-1 routes restored, CRF-2 SDK caller restored, CRF-3 checkExistingWorkspace moved back before validation, CRF-4 read_template returns the same blocked message. Netero re-scanned the round-2 diff and confirmed all 4 fixes, then found 2 new docs findings: CRF-5 (P2) and CRF-6 (P3). Both document the deployment-wide allowlist as the enforcement mechanism, which this PR made a no-op for chatd (enforcement now reads per-template agents_allowed; nothing reads the allowlist for enforcement). Per the Netero decision gate (pre-panel round, P2 present), this is a Netero-only COMMENT round; the panel does not run yet. This is the 2nd consecutive Netero-only round, so the next round forces the panel. Reviewed against 668c2e9..1df7b55. Round 3 updateBLOCKED. Diff content byte-identical to round 2 (PR rebased onto new base 890a45a; downstack DB migration PR). Churn guard classified CRF-5 (P2) and CRF-6 (P3) as silent: no code change to either docs file, and the author's deferral to a Round 4Churn guard PROCEED: CRF-5 (P2) and CRF-6 (P3) upgraded from silent to deferred-with-ticket. Author linked tracking PR #27518 and updated the PR description to link the full upstack (#27514, #27515, #27517, #27518). Diff content byte-identical to rounds 2-3 (another rebase onto base b80a335). Consecutive-Netero-only cap already reached in rounds 1-2, so Netero (step 3a) skipped; first panel round ran. Panel (16 reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Gon, Leorio, Chopper, ging-go, Kurapika, Knuckle, Ryosuke, Melody + wildcards Razor, Zoro). Verdict: the refactor is sound. All prior findings verified fixed. Enforcement is a least-privilege improvement (gate now runs under the chat owner's identity, replacing the broad AsChatd allowlist read), TOCTOU-safe (write path re-reads the row), and consistently applied across the three template-facing tools. 6 new findings: 1 P2 (CRF-7), 1 P3 (CRF-8), 4 Nits (CRF-9..12). Strong convergence (Hisoka, Mafuuu, Ryosuke, Pariston) that agents_allowed is a creation/discovery gate only: start_workspace is ungated and the idempotent create_workspace return runs before the gate, so existing workspaces on a blocked template stay usable/restartable while the SDK doc says the flag "controls whether Coder Agents can use this template." Orchestrator raised this to P2 on the combined consequence (reviewers rated P3/Note individually). Second convergence (Mafu-san, Knuckle, Ryosuke, Melody, Pariston) that the retained allowlist write surface is now a silent enforcement no-op, safe only if the stack merges atomically with #27514/#27515 (CRF-8); the downstack migration backfill (PR #27284, at base) preserves existing restrictions. Process finding (Mafu-san P3, folded into body): the "CI failures are base-origin" explanation has been carried unverified for 3 rounds while Round 5Churn guard PROCEED. Author pushed real code (diff grew to +386/-208, 88% test): CRF-7 fixed by narrowing both SDK doc comments to "can create workspaces using this template" (chose the narrower contract over gating start/reuse); CRF-9 const Round 6Churn guard PROCEED. One commit (0702d40) touching 3 test files: CRF-13 (blank lines) and CRF-14 (2 phase-label comments) fixed; CRF-15 accepted (author declined the shared-loader refactor as a follow-up). CRF-8 accepted, CRF-5/6 deferred. Proportionate re-review: Netero (advisory) + Bisky, Komugi, Gon + wildcard Zoro. Netero, Bisky, Komugi, Gon clean and confirmed the cosmetic edits dropped no struct field or needed line (go vet + gofmt clean). Wildcard Zoro found CRF-16 (Nit): the CRF-14 fix removed 2 of 3 sibling phase-label comments, leaving Round 7Churn guard PROCEED. One commit (9008f155) deleting a single line: the leftover About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
Documentation CheckThis PR makes the per-template Updates Needed
Note The PR description notes that the dashboard controls, the The auto-generated API reference changes ( Automated review via Coder Agents |
![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: 22cf8360e6
ℹ️ 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.
First-pass review only. These are mechanical findings from an automated first-pass reviewer; the full review panel has not yet reviewed this PR and will review after the finding below is addressed.
The change is well tested (70.6% test density) and the enforcement rewrite is clean: it replaces the experimental callback allowlist with a direct read of the persisted template.AgentsAllowed row, and the added API fields (create/update with default and preserve-on-omit semantics), telemetry, and agents-allowed: search key each have targeted tests. The allowlist->AgentsAllowed test/function/comment renames are consistent, and go build ./coderd/... ./codersdk/... succeeds.
Severity count: 1 P2, 3 Notes.
One finding should be addressed before the panel spends parallel review time: this PR removes the backend /template-allowlist routes but the frontend AI Settings > Templates page that calls them is still routed and unchanged, so on main at this PR's state that page 404s on both load and save. The PR body defers the frontend removal to an upstack PR, but as this PR stands alone the existing admin page is non-functional.
One process note: CI reports 4 failed jobs (test-go-pg, test-go-pg-17, test-go-race-pg, required). The first-pass reviewer could not attribute a failure to this PR's changed paths and lacked credentials to read the CI logs; the tests this PR touches passed on embedded Postgres. The failures need a look at the CI logs.
Netero, on the enforcement rewrite: "the change replaces the callback allowlist with a direct read of the existing template.AgentsAllowed row; no new helper duplicates an existing one."
codersdk/chats.go:2901
Note [CRF-2] The Go SDK methods GetChatTemplateAllowlist/UpdateChatTemplateAllowlist are now orphaned: their only caller (the deleted TestChatTemplateAllowlist) is gone, and they target the removed route. (Netero)
Pre-existing code, not modified here, so not dead-on-arrival, but they now point at a route that returns 404. Cleanup is deferred to the upstack PR per the description.
🤖
🤖 This review was automatically generated with Coder Agents.
22cf836 to
db7413e
Compare
|
@codex review |
|
/coder-agents-review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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.
First-pass (automated) round, panel not yet run. All four round-1 findings are verified fixed in db7413e: the /template-allowlist routes and their SDK caller are restored (CRF-1, CRF-2), checkExistingWorkspace runs before template validation again with a new regression test (CRF-3), and read_template now returns the same blocked message as create_workspace (CRF-4). Test density rose to 85%.
Two new findings this round, both about docs the enforcement change silently invalidated. Severity count: 1 P2, 1 P3.
The enforcement rewrite makes the per-template agents_allowed field authoritative and removes the deployment-wide allowlist from every chatd path (list_templates, read_template, create_workspace). The /template-allowlist routes are intentionally retained so the shipped AI Settings page keeps saving, but nothing reads the allowlist for enforcement anymore. Two published docs still tell admins the deployment-wide allowlist is how you restrict agent templates, so on main at this PR's state an admin who follows that guidance gets no restriction while the UI reports success. This should be addressed (or explicitly deferred with the behavior gap called out) before the panel spends parallel review time.
These are the mechanical floor; the full review panel reviews the feature code next round.
Netero, on the fix verification: "No re-raise of any closed finding."
docs/ai-coder/agents/platform-controls/template-optimization.md:27
P2 [CRF-5] The "Restrict available templates" how-to still documents the deployment-wide allowlist as the enforcement mechanism, which this PR made a no-op for chatd. (Netero)
This PR deletes Server.chatTemplateAllowlist and the AllowedTemplateIDs callbacks and makes list_templates, read_template, and create_workspace gate on the per-template agents_allowed column instead. Nothing reads the deployment-wide allowlist for enforcement anymore (grep confirms no chatd caller). The doc still tells admins that configuring the allowlist filters those three tools and that "the agent cannot see or provision templates that are not on the list" (lines 27-30), and that an empty allowlist means all templates are available (lines 32-33).
After this PR the allowlist setting has no effect on enforcement, and agents_allowed defaults to true, so an admin who follows this page to restrict agent templates gets no restriction and every template stays open to agents. The retained AI Settings page still writes the allowlist through the kept routes, which makes the stale instructions look live. The PR defers the doc rewrite to a future branch; either update this page here or call out the behavior gap explicitly, since enforcement already reads agents_allowed as of this PR.
🤖
docs/ai-coder/agents/platform-controls/index.md:84
P3 [CRF-6] The platform-controls overview repeats the stale allowlist enforcement claim: "the agent can only see and provision workspaces from the selected templates" and "when the allowlist is empty, all templates are available." (Netero)
Same root cause as CRF-5: these sentences describe the allowlist enforcement path that this PR routed through agents_allowed instead. Lower severity than the how-to because it is a summary rather than actionable configuration steps, but it makes the same false claim about how agent template access is controlled. Fix alongside the template-optimization.md update.
🤖
🤖 This review was automatically generated with Coder Agents.
|
Re CRF-5 and CRF-6 (stale allowlist docs): fixed in a new |
1df7b55 to
34588ac
Compare
668c2e9 to
890a45a
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Review blocked this round. The two round-2 findings are unaddressed in the code and the deferral has no tracking artifact, so the panel does not run until this clears.
The diff is byte-identical to round 2 (the PR was rebased onto a new base). Both prior findings are about docs that still describe the removed deployment-wide allowlist as the live enforcement mechanism:
- CRF-5 (P2)
docs/ai-coder/agents/platform-controls/template-optimization.md- the "Restrict available templates" how-to still tells admins the deployment-wide allowlist controls which templates agents can see and provision. After this PR nothing reads that allowlist for enforcement (chatd reads per-templateagents_allowed, defaulttrue), so an admin who follows this page gets no restriction while the UI still saves the setting. - CRF-6 (P3)
docs/ai-coder/agents/platform-controls/index.md- the overview repeats the same stale "empty allowlist = all templates" claim.
Your comment says the rewrite lives in a feat-docs-agents-allowed branch that merges atomically with the stack. That is a reasonable plan, but as posted it is a promise with no linked ticket or PR number, so it does not track. "The stack merges as a unit" is exactly the cross-PR assumption this review treats as absent: on main at this PR's state, enforcement already reads agents_allowed and these pages are already wrong.
To unblock, do one of:
- Push the doc updates into this PR.
- Link the follow-up PR number or a tracking issue in a comment (then this becomes a tracked deferral, not silent).
- State explicitly that these pages will ship stale for the interim and why that is acceptable (a human decision, which you can make as the author).
Process note, not a finding: CI reports 21 failed jobs (gen, fmt, lint, sqlc-vet, build, test-go-*, test-js, Storybook, and others). The diff content is identical to round 2, which passed most of CI, and the only change is the rebase onto base 890a45a, so the failures most likely originate from the new base rather than this PR's own changes. I could not confirm from the logs (no CI-log credentials). Worth a look before merge, since required is red.
🤖 This review was automatically generated with Coder Agents.
|
CRF-5 and CRF-6 are now tracked and addressed by #27518. That upstack PR updates both I've also updated this PR's description to link #27518 and the other upstack PRs explicitly. The stack is intended to merge as a unit, so the documentation ships with the completed feature. |
34588ac to
2a999e0
Compare
|
/coder-agents-review |
|
/coder-agents-review |
There was a problem hiding this comment.
Re-review after the round-4 fixes. This is in strong shape and the panel found nothing substantive. All six round-4 findings are resolved, accepted, or deferred, and five reviewers (Netero, Komugi, Mafuuu, Hisoka, Pariston) came back clean.
Verified this round:
- CRF-7 (P2): the narrowed SDK doc is honest. Both create and update fields now say the flag "controls whether Coder Agents can create workspaces using this template," which matches the enforcement (creation and discovery are gated; existing-workspace reuse and start_workspace are not, and neither is a creation). You chose the narrower contract and codified the idempotency invariant with a real test. Good resolution.
- CRF-9/10/11/12: const extracted and asserted verbatim on both sides, both fields documented with default/omit semantics, redundant authorization subtest replaced with a create-side table test, raw SQL removed. The dbgen preserve-seed default (from base #27284) is handled correctly: the author seeded true in exactly the tests that hit the gate and left the non-gate tests untouched.
Three new minor items, none blocking:
- CRF-13 (Nit): a stray blank line precedes
AgentsAllowed: truein ~10database.Template{}literals in listtemplates_test.go (and readtemplate_test.go:40), while sibling files inline it. gofmt tolerates both; collapse for consistency. - CRF-14 (Nit): two phase-label comments at chatd_test.go:11076 and :11082 (
// read_template: the blocked row fails...,// create_workspace: the blocked row is rejected.) restate therequire.Containsmessage on the line below them. Same class as the just-fixed CRF-12; worth deleting so the pattern doesn't creep back. - CRF-15 (Note): the
AgentsAllowedgate is enforced by caller discipline in three separate tools with no shared chokepoint. All three are correct today, but the next tool that resolves a template by ID for an agent must remember the check by hand (start_workspace already omits it, intentionally). A sharedloadChatTemplatehelper doing the org and AgentsAllowed checks once would eliminate the class. Structural suggestion, not a defect.
Still open outside the code:
- CI reports 21 failed jobs and
requiredis red. This round the evidence for base/infra origin is stronger: multiple reviewers built the changed packages and ran the changed tests green locally, gofmt/gofumpt is clean on every changed Go file, and the failing set spans fmt, lint-actions, offlinedocs, Storybook, test-js, and docs-preview, which this Go-only diff does not touch. But no one has confirmed it from the logs, and no in-thread triage note exists. Someone with CI-log access should confirm the failures reproduce on the base before merge. - CRF-8 (accepted, tracked by #27515) and CRF-5/CRF-6 (deferred, #27518) both rest on the stack merging as a unit. Not re-litigated.
Minor: the HEAD commit subject is just "review," which tells the next reader nothing, though it is discarded on squash-merge so it never reaches history.
Hisoka summed up the refactor: "The refactor replaced a deployment-wide allowlist callback with a per-row flag and lost nothing in the trade. Boring, and boring earns silence."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Re-review of the round-5 cleanup. CRF-13 (blank lines) and CRF-14 (phase-label comments) are fixed, and CRF-15 is reasonably declined (kept as a follow-up design consideration). Netero, Bisky, Komugi, and Gon all confirmed the cosmetic test edits dropped no struct field or needed line (go vet and gofmt clean, assertions intact).
One small precision miss from the wildcard reviewer: the CRF-14 fix removed two of the three sibling phase-label comments but left the third, so it now reads as an orphaned signpost. One-line fix, in the inline comment. Everything else is resolved, accepted (CRF-8, CRF-15), or deferred (CRF-5/CRF-6, tracked in #27518).
CI is green/pending this round after being red (21 jobs) across rounds 3-5. Since the diff was Go-only and reviewers repeatedly ran the changed tests green locally, that redness was almost certainly base/infra-origin, and it has now cleared. The remaining merge considerations are process, not code: the stack still needs to land as a unit (CRF-8/CRF-5/CRF-6 rest on that).
Zoro's take on the leftover comment: "a reader sees an orphaned signpost and expects two more that aren't there."
coderd/x/chatd/chatd_test.go:11070
Nit [CRF-16] The CRF-14 cleanup is incomplete: it removed the read_template and create_workspace phase-label comments but left the sibling // list_templates: only the allowed template should appear. (Zoro)
The two assertions below it already carry the same information in their messages ("allowed template should appear in list_templates result" / "blocked template should not appear in list_templates result"), which is exactly the restatement CRF-12 and CRF-14 targeted.
Worse than before the cleanup: the block now reads
// list_templates:with no matching// read_template:or// create_workspace:labels on the following blocks, so a reader sees an orphaned signpost and expects two more that aren't there. (Zoro)
Delete this line too; all three require blocks are self-describing through their assertion messages.
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9008f155aa
ℹ️ 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.
Converged. CRF-16 is fixed (the orphaned // list_templates: signpost is gone), and Netero, Bisky, and Zoro confirmed the deletion is clean with no struct field or assertion touched and no remaining restated-comment siblings of that class. All 16 tracked findings are now closed: fixed (CRF-1..4, 7, 9..14, 16), accepted (CRF-8 retained allowlist no-op and CRF-15 no-shared-chokepoint, both with reasoning), or deferred with tickets (CRF-5/CRF-6 docs, #27518).
The core change is sound and has been steady since round 4: the per-template agents_allowed column is the single authoritative gate, read directly under the chat owner's identity in list_templates, read_template, and create_workspace, with the deployment-wide allowlist callback deleted. Create defaults to true, update preserves on omit, the SDK docs honestly scope the flag to "create workspaces," and enforcement is well covered at 87% test density. CI has recovered to green/pending after the earlier base-origin redness cleared.
Approving the code. Two things remain outside the diff, both already decided by the author and not blockers to this review: the stack must merge as a unit (CRF-8 and the deferred docs rest on that), and one optional, take-it-or-leave-it comment trim below.
Optional (not tracked, reviewers split 2-1): Gon notes that at chatd_test.go:11000 the second sentence of the CreateWorkspace-stub comment ("The blocked template must be rejected before this function is called") restates the stub's own t.Error guard, the same class as the CRF-12/14/16 cleanups. Sentence 1 is a legitimate why-not-what and should stay. Netero and Zoro both evaluated the same comment and judged it acceptable as an ordering-invariant note, so this is a judgment call, not a defect; trim the second sentence if you agree, leave it if not.
Netero on the state of things: "All 16 prior findings are closed. No new evidence invalidates any closure."
🤖 This review was automatically generated with Coder Agents.
ba41499 to
2946853
Compare
9008f15 to
048b091
Compare
|
@codex review Re: Honor existing allowlist writes in chatd Declining for the same atomic-cutover reason. The old UI and endpoint exist only in the intermediate stack state. PR #27514 moves the UI to UpdateTemplateMeta, and PR #27515 removes the compatibility endpoint and stored allowlist before the stack ships. Bridging legacy writes would create a second authority for template access and could overwrite per-template changes. |
|
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". |
2946853 to
376f9a0
Compare
3fd69e5 to
6f99844
Compare
Relates to CODAGT-713 This adds `templates.agents_allowed` as a default-true, auditable template attribute, along with nullable database filtering. Migration `000562` translates the effective legacy `agents_template_allowlist` state 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.
6f99844 to
80be31f
Compare
2 participants
Relates to CODAGT-713
Depends on #27284
This makes the per-template
agents_allowedfield authoritative in the API and chatd. It adds optional create and metadata update fields with the intended default and omission semantics, supportsagents-allowed:template search, includes the value in telemetry, and makeslist_templates,read_template, andcreate_workspaceread the template row directly. Existing-workspace retries remain idempotent, and blocked same-organisation templates return an actionable message.The experimental
/template-allowlistroutes 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.