feat: notify users when chats are shared - #26914

Clean, well-scoped notification feature. The SQL recipient-diff query (new_readers EXCEPT old_readers with group expansion) is an elegant single-pass computation that keeps the complexity in the database. Test coverage is solid: positive assertions verify the exact recipient set with labels, targets, and createdBy, while negative assertions cover idempotent re-apply and unshare. The maps.Clone snapshot inside the transaction correctly captures pre-mutation state.

1 P1, 2 P2, 2 P3, 1 P4, 3 Nit.

The P1 blocks CI: TestMethodTestSuite requires a test entry for every dbauthz querier method, and GetChatShareNotificationRecipientIDs is missing. Three reviewers verified the failure independently.

The P2s are (1) initiator self-notification when sharing with a group the initiator belongs to, and (2) synchronous notification fan-out blocking the HTTP response for an unbounded duration when sharing with large groups. Both need a human decision: fix in this PR or file a ticket.

The PR description claims "the initiator is never self-notified," but the SQL query excludes owner_id, not initiator_id. These diverge when an admin (not the chat owner) shares a chat with a group they belong to. Admin users have ActionShare on ResourceChat at Site level, so this path is reachable today.

"The one fix that unblocks everything: add the dbauthz test entry. That's the pit stop the car missed." (Ryosuke)

🤖 This review was automatically generated with Coder Agents.

All 9 R1 findings addressed. The redesign from SQL-based recipient query to Go-based group expansion via effectiveChatReaders is structurally cleaner: deduplication falls out of the map, the initiator filter is explicit, and the owner is always in both reader sets so the diff naturally excludes them. The background goroutine on api.ctx fixes the HTTP latency and context cancellation issues. The new TestChatACLSharingExcludesGroupMemberInitiator test covers the non-owner initiator case and direct+group deduplication. All R1 fixes verified by 10 reviewers.

1 P3, 1 P4 new.

The P3 is about unbounded notification volume: sharing a chat with a large group (the "everyone" group in a 10k-member org) triggers ~10k enqueue calls from one PATCH request. The fan-out is bounded in concurrency (10 at a time) and runs in the background, so there's no correctness or latency issue. But the sustained DB load from a single user action is worth a human decision: accept the linear cost, cap recipients with a warning, or defer to a bulk notification API.

Minor note: the PR description's migration number (000534) doesn't match the actual migration file (000535). The CRF attributions in the description reference the superseded PR #26329's review and don't map to the current review's finding numbers.

"I tried to build a case against this change and couldn't. The problem is correctly understood, the solution is proportional, and the fix is at the right level." (Pariston)

🤖 This review was automatically generated with Coder Agents.

No new findings. CRF-11 (negative assertion synchronization) closed by panel vote: 5/5 reviewers verified the author's defense. The recipient diff for an unchanged or shrinking ACL is deterministically empty, making Enqueue unreachable regardless of goroutine timing. The assertion tests data correctness, not a timing window.

CRF-10 (unbounded notification volume) remains open for a human decision. The author's reasoning is sound: the fan-out is background, non-blocking, concurrency-capped, and the feature is experimental. Capping would silently drop legitimate notifications. A bulk-insert notification API would bound the cost but is a cross-cutting subsystem change. A human should decide: accept the linear cost, file a ticket for a bulk path, or close the finding.

11 findings across 3 rounds: 9 fixed, 1 closed by panel vote, 1 awaiting human disposition. The code is clean.

"The invariant is in the data, not the timing." (Meruem)

🤖 This review was automatically generated with Coder Agents.

All 11 findings across 4 rounds resolved. No new findings. 6 reviewers, unanimous.

The R4 design simplification is the cleanest iteration yet. directChatReaders replaces group expansion with a simple UserACL-only reader set, eliminating the unbounded fan-out (CRF-10) at the design level rather than capping it. No ResourceGroupMember:read escalation needed. Notification volume is now bounded by the ACL payload, which the caller controls.

The product tradeoff (group grants give access but don't trigger notifications) is well-documented in the PR description and tested explicitly in TestChatACLSharingNotifiesDirectReadersOnly.

"I tried to build a case against this change and could not. The problem is correctly understood, the solution is proportional, and the fix is at the right level." (Pariston)

🤖 This review was automatically generated with Coder Agents.

Writing critique of the PR text (title, description, comments, log messages, templates, docs). Every prose unit was enumerated and judged; findings below give the unit, the failure, and a full replacement. Inline comments carry the code-side and docs-side findings.

Finding 1: opening sentence is wider than the mechanism.
Unit: "Adds a Chat Shared system notification when a chat's ACL is updated."
Failure: the notification does not fire on every ACL update; the next paragraph itself says re-applying an unchanged ACL notifies nobody. The two claims conflict.
Replacement: "Adds a Chat Shared system notification when a chat ACL update grants a user direct read access."

Finding 2: "a client disconnect does not drop notifications" is oversized, in two places.
Unit: description bullet "Fan-out runs in a background goroutine on the server context (api.ctx), so it never blocks the PATCH response and a client disconnect does not drop notifications." and the code comment at exp_chats_acl.go:213 (inline comment there).
Failure: GetUserByID runs on the request context after the transaction commits and before the goroutine starts; a disconnect in that window cancels it and the whole fan-out is skipped with only a warning. Enqueue failures and server shutdown (api.ctx cancel) also drop notifications. The mechanism narrows the drop window; it does not eliminate drops.
Replacement bullet: "Fan-out runs in a background goroutine on the server context (api.ctx), so it does not block the PATCH response and a disconnect no longer cancels it once started. A disconnect between commit and the initiator lookup, or an enqueue failure, still drops the notification with only a warning log."

Finding 3: "ResourceGroupMember escalation" is a coinage that does not deliver its mechanism.
Unit: "No group expansion, so no ResourceGroupMember escalation and no AsSystemRestricted."
Failure: "escalation" stands in for a causal chain the reader must already know: expanding groups means reading group memberships, which the request actor may not be authorized for, which is what would force a system actor. The bullet names the consequence without the mechanism.
Replacement: "No group expansion, so the fan-out never reads group memberships and needs no AsSystemRestricted actor."

Finding 4: docs bullet duplicates a fact already stated.
Unit: "Docs updated (docs/ai-coder/agents/chat-sharing.md) to note direct shares are notified and group members are not."
Failure: the second paragraph already states who is and is not notified; restating it here puts the same fact in two places. The bullet's only new information is which file changed.
Replacement: "Docs updated (docs/ai-coder/agents/chat-sharing.md)."

Finding 5: second test description is a noun pile.
Unit: "TestChatACLSharingNotifiesDirectReadersOnly (non-owner org-admin group-member initiator sharing with a group plus a direct user notifies only the direct user)."
Failure: five stacked modifiers before "initiator" force the reader to re-parse; the actor and the action are buried.
Replacement: "TestChatACLSharingNotifiesDirectReadersOnly (an org admin who is not the owner and belongs to the shared group shares with that group plus one direct user; only the direct user is notified)."

Findings 6 and 7 are inline: the docs paragraph now conflicts with the unchanged step above it, and the fan-out warning log describes partial failure as total failure.

🤖 This review was automatically generated with Coder Agents.