Repository navigation
refactor(coderd): move chat and chat project ACL reads onto the shared helpers - #31016
ibetitsmike wants to merge 1 commit into
Conversation
…d helpers getChatACL and getChatProjectACL resolved users and groups with their own loops and counted group members with one query per group. Use resolveACLUsers and resolveACLGroups, which count every group in one query, and apply chat ACL updates with applyACLReadRoles.
|
@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. Already looking forward to the next diff. 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". |
Moves the chat ACL and chat project ACL reads onto
resolveACLUsersandresolveACLGroups, and the chat ACL update ontoapplyACLReadRoles. It deleteschatACLUsers,chatACLGroups,chatProjectACLGroups, and the user lookup inchatProjectACLUsers: 26 lines added, 174 removed. Both reads counted group members with one query per group; the shared resolver counts every group in one query.Both reads still list groups from every organization (
uuid.Nil), and the project read still drops users outside the project's organization before it resolves them. The resolvers' doc comments andnolintreasons now say that callers authorized reading the ACL, because these two reads check read access, not share.What changes: successful responses stay the same. A stored key that is not a UUID is skipped without a warning log,
500details use the shared texts, and an empty project ACL runs its lookups with empty ID lists instead of returning early.Why it is safe: #31014 pins the member counts and user fields of both reads. I ran 5 mutants of the changed lines with
go test -overlay: apply group roles to users or user roles to groups in the chat ACL update, keep project users from other organizations, filter project groups by organization, and drop the stored entry of project users. The tests fail on each.Review focus:
chatProjectACLUsers, which filters by membership and then resolves users through the shared helper.#29663 batches the same per-group count query in
coderd/exp_chats_acl.go, so the two conflict. The chat ACL update still writes request keys as sent, as onmain; #31037 canonicalizes them. It merges with this PR without conflicts, and whichever merges second will mergemainfirst.Validation: on
mainand on this branch,go test ./coderd -count=1passes with-run '(?i)acl|mcp|skill'(24 tests, 62 with subtests) and with-run '^(TestChatProjectSharing|TestChatSharingDisabled|TestListChatsSharedScope|TestSharedReaderStreamChat|TestChatFileDownloadURL)$'(5 tests, 22 with subtests), andgo test ./enterprise/coderd -run '(?i)acl|mcp|skill|TestEnterpriseAuditLogs' -count=1passes (9 tests, 46 with subtests). The pre-commit hook passed.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.