feat: add chat sharing foundation by DanielleMaywood · Pull Request #25041 · coder/coder

github-actions

The ACL persistence model, RBAC wiring, and write-path hardening are well-structured. The CHECK constraint chat_acl_only_on_root_chats enforcing ACL-inheritance at the schema level is a strong design choice. The four new ActionUpdate guards on mutation endpoints are consistent and correct. The owner-scoped markChatAsRead guard in streamChat is a thoughtful detail.

5 P2, 9 P3, 4 Nit, 1 Note.

The dominant theme is inconsistent ACL inheritance: three query families apply the CTE, three do not, and there is no type-level distinction between a Chat with effective ACLs and one with stored (empty) ACLs. GetChatsByWorkspaceIDs is user-facing and will deny shared readers access to sub-chats once the stacked PR adds shared-reader paths. Ryosuke's suggestion of a SQL VIEW to bake in inheritance as the default read path would eliminate this class of bug structurally.

The kill switch has two gaps: the SQL-filtered GetAuthorizedChats path unconditionally matches raw ACL columns (mirroring the same gap in workspace sharing), and the write path (UpdateChatACLByID) does not check ChatACLDisabled(), allowing ACL accumulation while sharing is disabled.

chatFromAutoArchiveRow in dbpurge does not map the new UserACL/GroupACL fields, producing audit records with nil ACLs for shared chats.

Process note: the PR body's implementation plan is a literal $(sed ...) shell expansion that does not render in GitHub markdown. The plan content is unrecoverable from the PR itself.

"Shall I show you?" (Hisoka, on the 16 call sites for GetChatByIDForUpdate that could one day reach shared readers on sub-chats)

Design notes for the stacked PR:

ActionShare is currently unreachable for regular users; only owner and orgAdmin can share. If owner-initiated sharing is intended, the agents-access role needs ActionShare.
has_unread reflects the owner's read position. Shared readers will see the owner's read state, not their own.
GetChatACLByID returns raw (stored) ACLs, not effective (inherited) ACLs. Sub-chats always return empty. The stacked PR's "show current ACLs" endpoint needs to resolve to the root chat.

Generated by Coder Agent (deep-review v0.4.8). 13-reviewer panel, round 1.

coderd/database/queries/chats.sql:1482

P2 [DEREM-5] GetChatsByWorkspaceIDs is the only user-facing read query that returns Chat objects without the ACL-inheritance CTE. Sub-chats have user_acl = '{}', group_acl = '{}' by constraint. When fetchWithPostFilter in dbauthz calls RBACObject() on these rows, the RBAC object carries empty ACLs, and any grant from the root chat is invisible. A shared reader who can access the root chat via inherited ACLs will be silently denied sub-chats through this path.

The handler at exp_chats.go:281 calls this through dbauthz in user context. Three of six Chat-returning query families apply the CTE; three do not. RBACObject() cannot distinguish which shape of Chat it received.

"The inconsistency is between these two groups." (Ryosuke)

A SQL VIEW that bakes in the COALESCE(root.user_acl, c.user_acl) join would make inheritance the default read path and make this class of bug structurally impossible. (Bisky P2, Hisoka P2, Mafuuu P2, Knov P2, Kite P2, Ryosuke P2, Knuckle P2, Kurapika P3)

🤖

coderd/database/dbpurge/dbpurge.go:460

P2 [DEREM-9] chatFromAutoArchiveRow stops at ClientType and does not map UserACL or GroupACL. The AutoArchiveInactiveChatsRow struct carries both as json.RawMessage, but neither is copied to the returned database.Chat. The audit table marks both fields as ActionTrack, so auto-archive audit records will show nil ACLs regardless of what the chat stored.

The fix follows the same Scan pattern used for Labels at line 425:

var userACL database.ChatACL
if err := userACL.Scan([]byte(r.UserACL)); err != nil {
    logger.Warn(...)
}
// same for GroupACL

(Chopper P2, Knov P3)

🤖

🤖 This review was automatically generated with Coder Agents.