Clean decomposition of a database foundation for chat ACL sharing. The schema design is sound: JSONB ACL columns on the chats table, CHECK constraints to enforce the root-only ACL invariant, and a view to resolve inheritance for child chats. The selective application of the view CTE (only to queries that can return child chats) shows good understanding of the inheritance model. Pattern reuse from WorkspaceACL is appropriate given the existing RBAC infrastructure.

1 P2, 5 P3, 3 nits, 2 P4, 1 note. The P2 is test coverage for the view's COALESCE inheritance logic, which is the core deliverable of this PR. The P3s are a mix of design awareness (ForUpdate ACL mismatch, missing view CTE on GetActiveChatsByAgentID), latent type safety (Value() nil marshaling), and untested constraints.

Several reviewers noted that the explicit 30-column enumeration in the view creates a maintenance coupling: every future column addition to chats requires a DROP/recreate migration for the view. This is inherent to PostgreSQL views with COALESCE overrides and will surface as a sqlc codegen failure, so the breakage is compile-time.

"A split-personality Chat struct where the data depends on the query that produced it, with no signal to the consumer." (Hisoka)

coderd/database/queries/chats.sql:1463

P3 [DEREM-6] GetActiveChatsByAgentID reads from the raw chats table without the chats_with_effective_acl view CTE. The query has no parent_chat_id IS NULL filter, so it can return child chats with empty ACLs instead of inherited ones.

Three other read queries (GetChatByID, GetChildChatsByParentIDs, GetChatsByWorkspaceIDs) were upgraded in this PR to use the view. This one was missed. The dbauthz wrapper uses fetchWithPostFilter, which will run RBAC checks against the returned Chat objects. When ACL enforcement arrives, child chats from this query will appear to have zero grants.

Fix: add the same WITH chats AS (SELECT * FROM chats_with_effective_acl) CTE. (Meruem)

🤖

🤖 This review was automatically generated with Coder Agents.

Good progress. All 7 R1 fixes verified by the panel: view inheritance tests, CHECK constraint rejection test, Value() nil guard, GetActiveChatsByAgentID CTE, Scan receiver pattern, roundtrip tests, and the view rename. The new tests are genuine (assert inherited values, not just "no error"; constraint test names the specific constraint; roundtrip tests exercise all Scan branches).

The panel unanimously closed all 4 contested findings (DEREM-4, DEREM-8, DEREM-10, DEREM-13). For DEREM-4 (ForUpdate mismatch): Chat.RBACObject() does not consume ACL fields, the upper stack is concrete (PRs 2-4 named in the description), and FOR UPDATE on a view would broaden lock scope. For DEREM-8 (dbpurge untested): audit diff is unaffected because before/after carry identical ACLs, and CHECK constraints guarantee valid JSONB. For DEREM-10 (no doc comment): the type signature is self-documenting and the project guidance supports omitting what-comments. For DEREM-13 (Scan nil): sibling types follow the identical pattern and coercing NULL would mask a broken invariant.

1 new P2 from the R2 changes: the GetChats query now materializes the full chats_expanded view on every call because the chats CTE is referenced twice (main query + cursor subquery). Before this PR, the cursor lookup was an indexed point read on the raw table. 1 nit.

"PostgreSQL materializes CTEs referenced more than once. This means every GetChats call evaluates the full chats_expanded view, stores the result in a temp table, then filters." (Hisoka)

🤖 This review was automatically generated with Coder Agents.

R2 fixes verified: the CTE materialization regression is fixed (cursor lookup now reads the raw table via a separate cursor_chat CTE), and tc := tc is removed. The structural restructuring to chats_expanded with owner fields is clean: lock semantics on GetChatByIDForUpdate are preserved, the ChatTable/Chat rename follows the established TemplateTable/Template pattern, and the new owner-field tests assert against real user data.

2 P3s this round.

coderd/database/gentest/models_test.go

P3 [DEREM-16] Missing TestViewSubsetChat for the new ChatTable/Chat type pair. Every other table/view pair has a gentest here (TestViewSubsetTemplate, TestViewSubsetTemplateVersion, TestViewSubsetWorkspaceBuild, TestViewSubsetWorkspace). This test prevents a future column addition to chats from silently diverging from chats_expanded. (Netero)

Meruem noted that the 16 inline CTEs duplicating the column list create a CTE-vs-view drift risk (distinct from the view-vs-table drift the gentest catches). The workspaces_expanded pattern avoids this by returning the raw type from write queries. Worth considering for a follow-up, but scope-extending for this PR.

coderd/database/querier_test.go:11509

P3 [DEREM-17] The 14 mutation return paths (ArchiveChatByID, UnarchiveChatByID, UpdateChatByID, etc.) that now return through the CTE+JOIN expansion lack assertions on OwnerUsername/OwnerName. Only 4 read paths are tested for owner field correctness.

All 16 JOINs are structurally identical by inspection, and sqlc catches column-count mismatches at generation. The real risk is low. But adding owner field assertions to the existing UpdateChatByID call here (2 lines) would cover the mutation CTE pattern with zero new fixtures:

require.Equal(t, owner.Username, chat.OwnerUsername)
require.Equal(t, owner.Name, chat.OwnerName)

(Bisky)

🤖

🤖 This review was automatically generated with Coder Agents.

All 17 findings from rounds 1-3 are resolved: 11 author-fixed, 4 panel-closed (contested), 2 author-accepted. R3 fixes verified: TestViewSubsetChat gentest guards against view/table column drift, owner-field assertions cover the representative mutation CTE path. Netero R4 and the panel (Bisky, Mafuuu, Meruem) found no new issues. CI green (18 passed).

This PR is a solid database foundation for chat ACL sharing. The schema design (JSONB ACL columns, CHECK constraints, view with COALESCE inheritance), the query architecture (selective view CTE application, cursor separation, inline CTE expansion for mutations), the type system (ChatTable/Chat split), and the test coverage (view inheritance, constraint rejection, ACL roundtrip, gentest subset guard, owner-field assertions) are all in good shape.

🤖 This review was automatically generated with Coder Agents.

Change looks OK to me once the migrations are fixed