feat(site): add per-template Coder Agents controls - #27514
Conversation
ca6e762 to
35387b5
Compare
34588ac to
2a999e0
Compare
|
@codex review |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 7 | Last posted: Round 7, 27 findings (6 P3, 11 Nit, 10 Note), APPROVE. Review Finding inventoryFinding inventory - PR #27514Findings
Contested and acknowledgedCRF-2 (P3, TemplatesPage.tsx:13) - Page gate requires editDeploymentConfig && updateTemplates
CRF-3 (Note, TemplatesPageView.tsx:95) - Switch controlled off server data, no optimistic update
CRF-5 (Nit, TemplatesPageView.tsx:177) - Error alerts detached from failing row
CRF-8 (Note, templates.ts:56) - Shared helper is the third wrapper
Round logRound 1Panel (16 reviewers: bisky, hisoka, mafu-san, mafuuu, pariston, nami, ging-react, ging-ts, komugi, gon, leorio, kite, chopper, robin, razor, meruem) plus Netero first pass. No Law (effective additions 432 < 1000). Reviewed against 2a999e0..35387b5. Netero: 1 P3, 1 Note. P3-and-below cleared the mechanical floor, panel proceeded. Cross-check resolution: Meruem rated the permission-gate issue P2 on the claim that the sidebar "Templates" link shows for Downgrade gate CRF-3 (Hisoka P3 -> Note): keep-at-P3 argument is that the controlled Switch stays visually frozen for the whole PATCH+refetch, so a slow link looks like a missed click. Downgraded because Process observations (CI fully red but not attributable to this diff; legacy SDK deferral to #27515; MixedOrganizations story-name overlap) surfaced in the review body, not as inline findings. New findings: CRF-1 (P3), CRF-2 (P3), CRF-3 (Note), CRF-4 (Nit), CRF-5 (Nit), CRF-6 (Nit), CRF-7 (Nit), CRF-8 (Note). Event: COMMENT (no P0/P1). Round 2Churn guard: PROCEED (4 addressed, 1 acknowledged, 3 contested, 0 silent). Reviewed against 4083e82..40740aa. CI now green. Between rounds the author also addressed two Codex findings not in this inventory (foreign IDs, not cross-referenced): a P1 on concurrent-toggle callbacks and a P2 on switch accessible names. The container Panel (14: bisky, hisoka, mafu-san, mafuuu, pariston, komugi, meruem, nami, chopper, ging-react, ging-ts, gon, leorio + wildcard zoro) plus Netero. Netero: no findings, all mechanical CRFs verified fixed. Panel verified the CRF-1 fix ( Drop gate CRF-12 (Gon P2 -> dropped): keep-argument is that the comment's first clause "Errors are tracked per template" restates the New findings: CRF-9 (Nit), CRF-10 (Note), CRF-11 (Note). Process observations in body: new stories added under a pre-existing Round 3Churn guard: PROCEED (3 addressed, 0 silent). Reviewed against 40740aa..4dcfdf6. CI green. CRF-9/10/11 fixed with real code: aria-label reworded (CRF-9); column header "Agents allowed" -> "New workspaces" and switch labels reworded to "Allow Coder Agents to create workspaces with ..." to scope the control to creation (CRF-10); and a URL-backed server-side Panel (15: bisky, hisoka, mafu-san, mafuuu, pariston, nami, robin, meruem, chopper, kite, komugi, leorio, ging-react, ging-ts + wildcard knov) plus Netero. Filter extraction verified as genuine reuse (Pariston, Mafu-san, Kite, Knov): the hook was lifted into the shared Cross-check severity: CRF-15 (Mafuuu Note -> P3). Keep-higher argument: the round-3 filter couples the per-row failure alert to the filtered CRF-13 (Netero P3, Mafu-san Note): held at P3. Pre-existing New findings: CRF-13 (P3), CRF-14 (Nit), CRF-15 (P3), CRF-16 (Nit), CRF-17 (Nit), CRF-18 (Note). Process note in body: commit subjects "review"/"review" uninformative (Leorio), harmless under squash-merge. Event: COMMENT (two open P3s, no P0/P1). Round 4Churn guard: PROCEED (6 addressed, 0 silent). Reviewed against 7713788..33c31a7. CI red but stack-inherited: 20/20 jobs red including Go/sqlc/offlinedocs which a frontend diff cannot touch; orchestrator verified biome, tsc, and the changed stories' play functions pass locally (the 3 storybook failures are an unrelated xterm error in AgentChatPageView.stories.tsx). CRF-13/14/15/16/17/18 all fixed with real code. CRF-15 fix: alerts now render from Open risk to confirm (not a code defect): CRF-13's fix removed the Panel (15: bisky, hisoka, mafu-san, mafuuu, pariston, nami, robin, meruem, chopper, komugi, kite, leorio, ging-react, ging-ts + wildcard zoro) plus Netero. 11 no-findings. All round-3 fixes verified genuine: CRF-15 decoupled (alerts render from New CRF-19 (Note, convergent Mafuuu/Nami/Meruem): the CRF-15 fix's flip side. Because the error is a snapshot cleared only by re-toggling that row's Switch, (a) a failure whose row is filtered out has no in-page retry/dismiss until the filter clears or a reload, and (b) a lost-response success (PATCH succeeds server-side, client sees error) leaves a stale red alert while the refetched Switch shows success, so alert and switch contradict. All three rated it Note: bounded, recoverable, and the deliberate visible-over-hidden tradeoff the panel endorsed at CRF-15. Meruem's structural option: derive the alert from live query state (desired value vs refetched agents_allowed) so a confirmed success retires its own alert. Pre-existing/out-of-scope (body only, not CRFs): Komugi Note (ServerSideFilter 500ms debounce inside 1000ms waitFor, unchanged since R3, accepted); Mafuuu P4 ( Event: COMMENT. No code defect blocks, but the hosted Storybook/pixel job on this head is unverified and must be confirmed green before merge (CRF-13 removed the exclude that masked pixel failures), so not APPROVE. Round 5Churn guard: PROCEED (1 addressed, 0 silent). Reviewed against 33c31a7..8bff9a9. CI now green (Storybook passes on this head, resolving the round-4 pixel confirmation). Two new production surfaces this round, both reviewed fresh:
Panel (16: bisky, hisoka, mafu-san, mafuuu, pariston, nami, meruem, robin, chopper, komugi, knov, kite, leorio, ging-react, ging-ts + wildcard razor) plus Netero. 10 no-findings. Both new surfaces verified sound by multiple reviewers: the toast pivot resolves both CRF-19 halves (transient dismissible toast decoupled from the row; ConcurrentToggles story proves it survives filtering), and the cache reconciliation's New findings (nothing blocking): CRF-20 (Nit, description drift), CRF-21 (Note, convergent Meruem/Chopper/Knov/Leorio: isTemplateListQuery unenforced-invariant future-fragility, correct today), CRF-22 (Note, Komugi: toast-survival assertion pinned only by the 4000ms auto-dismiss; Body-only (pre-existing/out-of-scope/endorsed, not CRFs): TemplateSchedulePage's hand-rolled updateTemplateMeta wrapper not brought into the list-reconciliation contract (Mafu-san/Pariston; CRF-8-adjacent, non-hazardous since it never changes agents_allowed); Robin's note that chats.ts has a similar isChatListQuery/updateInfiniteChatsCache pattern (correctly NOT abstracted, different cache shapes); Bisky's note that the toast Event: APPROVE (auto-downgraded to COMMENT). All 19 prior findings fixed or settled, CI green (pixel confirmation resolved), new items are non-blocking Notes/Nits. Round 6Churn guard: PROCEED (4 addressed, 0 silent). Reviewed against 61e84fb..715c27a. CI red but stack-inherited again (20/20 including Go/sqlc/offlinedocs; base rebased to 61e84fb). Orchestrator verified biome + tsc clean on the changed files; no hardcoded old query keys remain. All 4 round-5 findings fixed. Main new surface: the CRF-21 fix took the stronger mitigation, an explicit Panel (16: bisky, hisoka, mafu-san, mafuuu, pariston, nami, meruem, robin, knov, kite, chopper, komugi, leorio, ging-react, ging-ts + wildcard zoro) plus Netero. 11 no-findings. The CRF-21 key-hierarchy fix verified as the correct structural cure by ~8 reviewers (discriminant New findings: CRF-24 (P3, Hisoka), CRF-25 (P3, convergent Zoro/Hisoka/Chopper), CRF-26 (Nit, Zoro). CRF-24 severity: P3. The CRF-25 severity: Zoro P3, Hisoka/Chopper Note -> held at P3. Keep-higher: the reachability-dismissal rule says do not downgrade an exported function's finding because no current caller hits it; this helper is exported and CRF-8's defense positioned it as the canonical migration target, so a future rename caller silently regresses the new-name detail cache, a bug the replaced General Settings code explicitly guards. Latent today; one-line fix ( Body-only (pre-existing/out-of-scope/carryover, not CRFs): the standing Event: COMMENT (two open P3s, no P0/P1). Round 7Churn guard: PROCEED (3 addressed, 0 silent). Reviewed against 0702d40..ef6c531. CI red but stack-inherited again (base rebased; 20/20 including Go/sqlc/offlinedocs). All 3 round-6 findings fixed with real code:
Panel reviews the CRF-24 error-rendering restructure (edge cases: refetch-with-cache vs initial-no-data vs validation error) and the CRF-25 rename-safe invalidation fresh. Panel (14: bisky, hisoka, mafu-san, mafuuu, pariston, nami, meruem, chopper, knov, kite, komugi, ging-react, ging-ts + wildcard razor) plus Netero. 12 no-findings. Both fixes verified sound: the three CRF-24 render states behave correctly (Pariston/Meruem traced refetch-with-cache -> alert+rows, initial-no-data -> alert only, validation -> table hidden + filter error, no double-error; Pariston notes the initial-error handling is actually cleaner than the sibling /templates page's permanent skeleton), and CRF-25's rename-safe invalidation matches the General Settings sibling with a test that pins the exact regression (fails on the pre-fix code). CRF-26 trivial. New findings (both Notes, non-blocking): CRF-27 (Bisky, the 304 fallback branch is untested, P4-equivalent), CRF-28 (Pariston, structural observation that four findings share the parallel-list-view root; explicitly not a refactor request). Mafu-san correction incorporated: the CI-red set also includes frontend-relevant jobs (test-js, Storybook, lint, fmt, gen), not only Go/sqlc/offlinedocs; he ran the changed unit tests, tsc, biome, and all 17 changed-story play functions locally and they pass, so the red is not attributable to this diff's changed files. Stack-inheritance stands, verified more broadly this round. Event: APPROVE (auto-downgraded to COMMENT). All 26 prior findings fixed or settled; the two round-6 P3s are resolved; new items are non-blocking Notes. Law analysisNot run (effective additions 432 R1, 552 R2, 733 R3, 864 R4; all < 1000 threshold). 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: 35387b5d50
ℹ️ 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.
Clean, net-negative migration: it swaps the deployment-wide allowlist picker for a per-template agents_allowed toggle across three surfaces (AI Settings > Templates, template general settings, sidebar nav) and adds a shared updateTemplateMeta mutation. The strongest part is the per-row state design: pending and error state are keyed by template ID in component-owned Set/Map with functional updaters and per-mutate onError/onSettled callbacks, so concurrent row toggles never clobber each other. Reviewers checked the backend contract end to end and confirmed the single-field PATCH is safe: resolveTemplateMetaUpdate falls back to the existing template for every nil field, so sending only { agents_allowed } does not wipe name, description, TTLs, or ACLs. Permission deny-paths and the two-error MutationError case are covered by stories, not just the happy path.
Severity count: 2 P3, 2 Note, 4 Nit. No P0/P1/P2.
One cross-check worth stating: the permission-gate finding was raised at P2 on the premise that the sidebar "Templates" link and the page gate disagree and leave a reachable dead-end. That premise does not hold. In AISettingsSidebarView.tsx the link (lines 65-67) is nested inside the permissions.editDeploymentConfig block and additionally gated on updateTemplates, so it renders only when editDeploymentConfig && updateTemplates, exactly matching the page's canManageTemplates. Sidebar and page are in sync. What survives is narrower and is filed as CRF-2 at P3: the gate requires editDeploymentConfig even though the operation is a template:update, so it is stricter than both the backend authz and the PR description ("limited to users who can update templates"). That is a design + description question a human should settle.
Process notes, not inline findings:
- CI is fully red (20/20 required jobs, including
fmt,lint,gen,offlinedocs, and unrelated Go jobs), but four reviewers independently rantsc --noEmit,biome, and the changed story tests locally and all pass on this diff's files. The redness is consistent with the stacked dependency ("Depends on #27285") running against a base that lacks it, not with a defect in this diff. Please confirm the red is stack-inherited and clears once #27285 is in the base; "red because stacked" is currently an inference, not a verified fact. - The legacy
API.getChatTemplateAllowlist/updateChatTemplateAllowlistSDK methods lose their last callers here and now target routes #27285 already deleted. The PR body defers their removal to #27515, so this is a deliberate split boundary, not a silent drop, worth confirming #27515 actually lands. - Minor: the
MixedOrganizationsstory name promises grouping the view does not do (it renders three rows with different org labels). Worth knowing, not worth changing.
Hisoka, after trying to break the concurrency: "I tried to make it drop a row's error or leak another row's pending flag. It wouldn't. Nice work."
🤖 This review was automatically generated with Coder Agents.
2a999e0 to
4083e82
Compare
35387b5 to
616f118
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.
Round 2 lands the round-1 feedback cleanly and the new production code holds up. The concurrency refactor is the substantive change: toggleAgentsAllowed now awaits each mutateAsync in its own try/catch/finally with pending and error state keyed per template ID, so a shared useMutation observer can no longer clobber a row (this also resolves the Codex P1 on per-call callbacks). The panel traced it end to end and it is correct: the switch is disabled synchronously before the first await (no double-fire), success resolves only after the invalidation refetch settles (no snap-back flicker), and per-row errors neither overwrite nor clear each other.
The CRF-1 fix is a real interaction test, not a token render check. The new ConcurrentToggles story drives two overlapping toggles through deterministic deferreds, rejects one while resolving the other, asserts the failing row keeps its scoped alert while the succeeding row updates, then retries and asserts the alert clears. Komugi cleared it for flake specifically: ordering rests on sequential awaits and the onSuccess -> invalidate -> refetch happens-before edge, not scheduling luck.
Round-1 disposition: CRF-4, CRF-6, CRF-7 fixed and verified. CRF-2 (dual permission gate) was the one item needing a human decision; the author made it (deployment-admin scoping is intentional, template admins still edit the field per-template from General Settings) and aligned the PR description, and the sidebar/page gates match with no reachable dead-end, so the panel accepts it. CRF-3 (non-optimistic switch) and CRF-5 (below-table alerts) remain conscious, proportionate tradeoffs; CRF-8 stays a correctly scoped-out migration.
New this round: 1 Nit, 2 Notes, nothing blocking. The two Notes are product judgment calls for a human, not defects: the binary "Agents allowed" switch only gates creation and discovery (the copy is honest about this), and the bulk table dropped the searchable picker for an unsorted, unfiltered wall of switches that scales poorly on large multi-org deployments.
Process notes: the round adds several stories into a file carrying a pre-existing pixel: { exclude: true } with a vague unticketed TODO, so the Templates table has no visual-regression coverage; worth a human deciding whether the pixel-harness failure gets a ticket before more stories pile up behind the exclude (the container stories are not excluded, so the concurrency test does run). The fix commit subject is the single word "review", which is uninformative, but Coder squash-merges so the PR title replaces it on main.
Bisky on the new test: "It fires two toggles while both mutations sit unresolved on deferreds ... Deterministic deferreds, no time.Now(), no wall-clock. This tests the exact concurrency the container refactor introduced, not a mock echoing itself."
🤖 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: 4dcfdf6fc7
ℹ️ 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.
Round 3 lands the round-2 items with real code and the new surface mostly holds. CRF-9 (aria-label reworded), CRF-10 (column renamed "New workspaces", switch labels scoped to creation), and CRF-11 (URL-backed server-side filter) are all fixed. The CRF-11 fix is genuine reuse, not a fork: the useTemplatesFilter/TemplateFilterState hook was lifted out of the general /templates page into the shared TemplatesFilter module, both pages now import one definition, and the added enabled option defaults to true so /templates behavior is unchanged while the AI Settings page threads enabled: canManageTemplates into both the filter user-menu and the templates query. Partial-PATCH safety, the dual permission gate, and query invalidation were all re-verified against the backend.
Severity count: 2 P3, 1 Note, 3 Nit. No P0/P1.
The one worth fixing before merge is CRF-15: the new filter coupled the per-row failure alert to the filtered result set, so filtering out a failed row, or a filter validation error that blanks the whole results body, hides an active update failure. Because the switch is non-optimistic (CRF-3, it never moves on click), a filtered-out failed toggle can show no feedback at all, and that bites exactly the large-list workflow the filter was added for. The signal is recoverable (it returns when the filter clears), which is why Mafuuu filed it as a Note, but hiding a write failure during the feature's own workflow is a P3 in my read. Keying the alerts off updateErrors directly rather than the filtered templates list closes it.
The rest are minor: the CRF-10 reword didn't reach the sibling per-template General Settings checkbox (three reviewers), a missing period on the filtered-empty message, and some row markup that could reuse the shared AvatarData and developer-count helper the sibling page already uses. CRF-13 (the pre-existing pixel: { exclude: true } with an unticketed TODO, now carrying more stories each round) is held at P3 and needs a human decision: file a ticket or fix the play functions, not silent acceptance. Interaction coverage still runs in the normal Storybook runner; only visual-regression/pixel coverage is lost.
Process note: the last two commit subjects are both "review", which is uninformative, though squash-merge under the PR title makes it harmless on main.
Hisoka, on the fresh filter surface: "I came to fight the fresh round-3 surface and it held. Boring in the best way. No strike to throw."
🤖 This review was automatically generated with Coder Agents.
4083e82 to
7713788
Compare
4dcfdf6 to
33c31a7
Compare
|
@codex review |
|
/coder-agents-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33c31a7550
ℹ️ 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: Filter task templates by agent allowance Thanks, but this is not a bypass of the new control. agents_allowed replaces the legacy chat template allowlist and applies to the Coder Agents list_templates, read_template, and create_workspace tools. TasksPage uses the separate Tasks API, which the legacy allowlist did not control. The story change only updates the React Query cache key to match the existing production query. Adding only agents-allowed:true to the frontend query would also be insufficient enforcement. A client could still call the Tasks API directly. If this setting should also govern the Tasks API, that requires a separate backend-first change with API tests, followed by the frontend filter and story updates. Closing this as out of scope. |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
73e9ab6 to
e1df7ab
Compare
DanielleMaywood
left a comment
DanielleMaywood
left a comment
There was a problem hiding this comment.
Looks mostly okay! I'm not the biggest fan of the useTemplatesFilter hook but I'll be honest and say I'm not 100% sure what the alternative looks like.
34696f8 to
6cd0526
Compare
048b091 to
3fd69e5
Compare
6cd0526 to
ccd94e6
Compare
3fd69e5 to
6f99844
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.
ccd94e6 to
35c2c38
Compare
2 participants
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_allowedto 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-allowlistAPI. #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.