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.

Strong response to R1. All 5 P2 findings addressed (4 in this PR, 1 in #25080). All 3 contested nits closed by unanimous panel vote (6/6 accept defenses). The PR split into #25080 (database foundation) + this PR (behavior) resolves the CTE duplication and ACL inheritance inconsistency structurally.

0 P2, 4 P3, 1 P4.

The kill switch now enforces at three layers (RBAC object, SQL filter, dbauthz guard). The canReadLinkedChat helper with LIMIT 1 replaces the unbounded iteration cleanly. Write-path hardening covers all mutation handlers and has rejection tests.

Two testing gaps remain: the GetAuthorizedChatsByChatFileID SQL path (file access for shared readers) is tested only through mocks, and the group ACL SQL matcher has no real-DB coverage. Both are fail-closed (over-deny, not over-grant), so the risk is degraded UX, not a security hole.

One behavior inconsistency: regenerateChatTitle and proposeChatTitle return 404 (not 403) to shared readers who demonstrably have read access. The other four hardened handlers return 403.

"One real-DB test that creates a chat, shares it via ACL, links a file, and reads that file as the recipient would nail this shut." (Hisoka)

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

coderd/exp_chats.go:3486

P3 [DEREM-24] regenerateChatTitle and proposeChatTitle return httpapi.ResourceNotFound(rw) on ActionUpdate denial. The four handlers hardened in this PR (patchChat, patchChatMessage, deleteChatQueuedMessage, interruptChat) all return httpapi.Forbidden(rw). A shared reader who can stream and read messages gets 404 for title operations, implying the resource doesn't exist, contradicting the read path that just served it. 403 is the correct response for a reader who lacks update permission on a resource they demonstrably have read access to.

Fix: change both handlers to httpapi.Forbidden(rw) to match the pattern this PR establishes. (Mafuuu P3)

🤖

🤖 This review was automatically generated with Coder Agents.

All R2 findings resolved. DEREM-23 (markChatAsRead guard untested) closed by unanimous panel vote (3/3 accept defense). The dbauthz layer enforces ActionUpdate on UpdateChatLastReadMessageID, so the stream guard is defense-in-depth preventing log noise, not a correctness boundary.

0 P2, 1 P3, 1 P4.

The test suite is now comprehensive: real-DB coverage for user ACL sharing, group ACL sharing, linked-chat file authorization, kill switch denial, and the OwnedOnly/SharedOnly/ViewerID validations. TestChatReadOnlySharedWriteHandlers covers 6 of 9 write handlers; the remaining 3 (PostChatMessages, PromoteChatQueuedMessage, PostChatToolResults) are protected by the same ActionUpdate check but lack explicit shared-reader coverage.

27 findings across 3 rounds: 5 P2, 14 P3, 2 P4, 3 Nit, 1 Note. 22 fixed, 4 contested and closed by panel vote, 1 acknowledged. 2 open (P3 + P4).

"No worthy opponents this round." (Hisoka)

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

🤖 This review was automatically generated with Coder Agents.

All 27 findings across 4 rounds resolved. Zero open findings.

22 fixed by author, 4 contested and closed by panel vote, 1 acknowledged as intentional scaffolding.

The PR delivers a solid chat sharing foundation: three-layer kill switch enforcement (RBAC object, SQL filter, dbauthz guard), consistent write-path hardening across all 9 mutation handlers with shared-reader rejection tests, linked-chat file authorization with hoisted SQL filter preparation, and comprehensive real-DB test coverage for user ACL, group ACL, and file-link authorization paths.

Generated by Coder Agent (deep-review v0.4.8). 4 rounds, 27 findings.

🤖 This review was automatically generated with Coder Agents.