Repository navigation
refactor(coderd): move chat model ACL handlers onto the shared ACL helpers - #31015
ibetitsmike wants to merge 1 commit into
Conversation
…lpers The chat model ACL read, available, and update handlers kept their own copies of the user and group resolvers, the member count query, the organization checks, and the key canonicalization that coderd/mcp_acl.go already shares between MCP servers and organization skills. Use the shared helpers and delete the copies. resolveACLUsers and resolveACLGroups now pass each stored ACL entry to the callback so chat roles can be read from it, and resolveACLGroups takes an organization ID (uuid.Nil keeps every organization) so the model ACL still hides groups from other organizations.
|
@codex review
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65c242b762
ℹ️ 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".
Moves the chat model ACL handlers onto the organization ACL helpers in
coderd/mcp_acl.go, which MCP server configs and organization skills already share. It deletes the model copies of the user and group resolvers, the member count query, the list of available principals, the organization and duplicate ID checks, and the role update loop: 30 lines added, 321 removed.Two shared helpers change:
resolveACLUsersandresolveACLGroupspass each stored ACL entry to the callback, so the model ACL reads each role from it.resolveACLGroupstakes an organization ID. The model ACL passes its organization, so groups from other organizations stay hidden. MCP server configs and organization skills passuuid.Nil, which keeps every organization, as today.What changes: successful responses stay the same. On error paths, a stored key that is not a UUID is skipped without a warning log (
PATCHvalidation rejects such keys),500details use the shared texts (get ACL usersinstead ofhydrate chat model ACL users), duplicate ID errors come back in the same400as the other validation errors because the check now runs inside the write transaction, and a failed membership lookup skips the group check.Why it is safe:
TestChatModelACLchecks that groups from other organizations stay hidden, andTestChatModelACLValidationIsAtomiccovers the duplicate check. I ran 9 mutants of the changed lines withgo test -overlay: ignore the organization inresolveACLGroupsor passuuid.Nilfrom each handler, drop the stored entry given to either callback, report zero members, and apply group roles to users or user roles to groups. The tests fail on each.Review focus:
uuid.Nilmeans every organization inresolveACLGroups, as it does inGetGroups.Validation: on
mainand on this branch,go test ./coderd -run '(?i)acl|mcp|skill' -count=1passes (24 tests, 62 with subtests) andgo test ./enterprise/coderd -run '(?i)acl|mcp|skill|TestEnterpriseAuditLogs' -count=1passes (9 tests, 46 with subtests). The pre-commit hook passed (make gen, formatting, golangci-lint, slim build).Stack
This stack moves the chat, chat project, and chat model ACL handlers onto the organization ACL helpers that MCP server configs and organization skills share, and deletes their copies. Successful ACL responses do not change.