From 51c626db1fe29f4a1646345e64b83d4b4637d5bd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 01:53:52 +0000 Subject: [PATCH 01/14] docs: spec for RoomType on Subscription (rebased on main) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resets the spec onto current main: PR #118 already removed RoomTypeGroup and renamed broadcast-worker's switch case, role-update guard, and errRoomTypeGuard sentinel — those scope items are dropped. PR #118 also deleted processInvite, so the invite-side population step is gone too. Surviving work: the new constants, the Subscription.RoomType field, and populating it on three sub-creation sites and two remove-event payloads. --- ...6-04-23-roomtype-on-subscription-design.md | 160 ++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100644 docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md diff --git a/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md b/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md new file mode 100644 index 000000000..4bdc7cea1 --- /dev/null +++ b/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md @@ -0,0 +1,160 @@ +# Design: RoomType on Subscription + +## Summary + +Add a `RoomType` field to `model.Subscription` so that every subscription record +carries the kind of room it belongs to. Align the `RoomType` enum with the +product's current room taxonomy: `dm`, `channel`, `botDM`, `discussion`. +Remove the legacy `RoomTypeGroup = "group"` constant and replace every +call site with `RoomTypeChannel`. + +## Motivation + +Today, any consumer that needs to know whether a subscription belongs to a DM, +channel, bot DM, or discussion must look up the room document. Denormalising +the room type onto the subscription lets consumers decide how to handle events +(UI rendering, notification routing, fan-out strategy) using only the +subscription payload. The rename from `group` to `channel` follows the +product's current naming. + +## Scope + +### In scope + +1. `pkg/model/room.go` — enum changes. +2. `pkg/model/subscription.go` — new field. +3. All production code that creates a `model.Subscription` — populate the new + field. +4. All production code and tests that reference `model.RoomTypeGroup` — rename + to `model.RoomTypeChannel`. +5. Unit and integration tests updated accordingly. + +### Out of scope + +- Data migration/backfill of existing subscriptions. Old Mongo documents keep + `RoomType: ""` until they are rewritten by normal traffic or a separate + backfill script (not part of this PR). +- New fan-out behaviour for `botDM` or `discussion` in `broadcast-worker` — + those types fall through to the existing default warning branch. +- Any UI/client changes. + +## Design + +### 1. `pkg/model/room.go` + +```go +const ( + RoomTypeChannel RoomType = "channel" + RoomTypeDM RoomType = "dm" + RoomTypeBotDM RoomType = "botDM" + RoomTypeDiscussion RoomType = "discussion" +) +``` + +- Remove `RoomTypeGroup`. +- Add `RoomTypeBotDM` and `RoomTypeDiscussion`. +- `RoomTypeChannel` and `RoomTypeDM` are unchanged. + +### 2. `pkg/model/subscription.go` + +Add `RoomType` to the `Subscription` struct between `RoomID` and `SiteID`: + +```go +type Subscription struct { + ID string `json:"id" bson:"_id"` + User SubscriptionUser `json:"u" bson:"u"` + RoomID string `json:"roomId" bson:"roomId"` + RoomType RoomType `json:"roomType" bson:"roomType"` + SiteID string `json:"siteId" bson:"siteId"` + Roles []Role `json:"roles" bson:"roles"` + HistorySharedSince *time.Time `json:"historySharedSince,omitempty" bson:"historySharedSince,omitempty"` + JoinedAt time.Time `json:"joinedAt" bson:"joinedAt"` + LastSeenAt time.Time `json:"lastSeenAt" bson:"lastSeenAt"` + HasMention bool `json:"hasMention" bson:"hasMention"` +} +``` + +### 3. Subscription creation sites + +| Site | Source of `RoomType` | +|---|---| +| `room-service/handler.go` `handleCreateRoom` (owner subscription) | `req.Type` (the type the room is being created as) | +| `room-worker/handler.go` `processAddMembers` | Hardcoded `RoomTypeChannel`. DM and botDM do not support add-member. | +| `inbox-worker/handler.go` `handleMemberAdded` | Hardcoded `RoomTypeChannel`. Cross-site member_added events only fire for rooms that support add-member. | + +`processInvite` is intentionally absent: PR #118 ("Remove-member / +role-update hardening", merged Apr 24 to main) removed the single-user +invite flow entirely. The `.member.invite` subject is no longer wired up +in `HandleJetStreamMsg`, and the function does not exist on main. + +### 4. Partial `Subscription` payloads on removed events + +In `room-worker/handler.go` the "removed" variants of `SubscriptionUpdateEvent` +construct a partial `Subscription` literal. Populate `RoomType` by fetching the +room once per operation (not per account): + +- `processRemoveIndividual`: call `store.GetRoom(ctx, req.RoomID)` + near the top of the function, reuse `room.Type` when building the + `SubscriptionUpdateEvent.Subscription`. +- `processRemoveOrg`: same — one `GetRoom` at the top, reuse across + the per-account event loop. + +If `GetRoom` returns an error, log at warn level and continue with +`RoomType: ""`. The removal itself must not block on this lookup; populating +the field is best-effort for the notification payload. + +### 5. `RoomTypeGroup` → `RoomTypeChannel` renames + +PR #118 already removed `RoomTypeGroup` from `pkg/model/room.go`, renamed +`broadcast-worker`'s switch case + `publishGroupEvent` → `publishChannelEvent`, +and updated `room-service`'s role-update guard plus the `errRoomTypeGuard` +sentinel message. No additional renames are required in this change. + +### 6. Store interfaces + +No new store methods required. `room-worker` already has `GetRoom` in its +`SubscriptionStore` interface. + +## Test plan + +Following the repository's TDD rules: write failing tests, then implement. + +- `pkg/model/model_test.go` + - Update `TestRoomTypeValues`: assert the new set of constants. + - Add a `Subscription` round-trip test case that sets `RoomType` and + confirms it survives JSON and BSON marshal/unmarshal. +- `room-service/handler_test.go` + - `TestCreateRoom`: assert the subscription captured by the store mock has + `RoomType` equal to the request's `Type`. +- `room-worker/handler_test.go` + - `processAddMembers`: assert each created subscription has + `RoomType: RoomTypeChannel`. + - `processRemoveIndividual`: assert the published `SubscriptionUpdateEvent` + payload has `Subscription.RoomType` equal to the fetched room's type. + - `processRemoveOrg`: same assertion across the per-account events. +- `inbox-worker/handler_test.go` + - `handleMemberAdded`: assert each created subscription has + `RoomType: RoomTypeChannel`. +- Run `make generate` to refresh mocks (no interface changes expected, but run + for safety). +- `make lint`, `make test`, `make test-integration` must pass. + +## Commit strategy + +A single commit on branch `claude/add-roomtype-subscription-Uqow3` covering +the model change, all call-site updates, renames, and tests. The rename is +atomic — splitting across commits would leave the tree unbuildable between +commits. + +## Risks + +- **Old subscriptions without `roomType`.** Returned as `RoomType: ""`. No + code currently reads the field, so this is harmless until a consumer starts + relying on it. A future backfill job (out of scope) can populate old + records. +- **Client code consuming `SubscriptionUpdateEvent.Subscription.RoomType` on + removed events.** Covered by populating the field from the fetched room; if + the fetch fails the field is empty, same as before the change. +- **Silent data drift** if a future creation site forgets to set + `RoomType`. Mitigated by the test plan above, which asserts `RoomType` on + every known creation path. From ffe6e1175fa0033794c74e3e891fa0a3cc1ab7b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 01:55:41 +0000 Subject: [PATCH 02/14] model: add RoomType on Subscription and botDM/discussion constants Adds the new RoomTypeBotDM and RoomTypeDiscussion constants and a RoomType field on Subscription so that downstream consumers can route on room kind without an extra room lookup. The TestSubscriptionJSON round-trip now exercises the new field; TestRoomTypeValues asserts each of the four constants. --- pkg/model/model_test.go | 7 +++++++ pkg/model/room.go | 6 ++++-- pkg/model/subscription.go | 1 + 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index 4f945a828..fc3325bbd 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -384,6 +384,7 @@ func TestSubscriptionJSON(t *testing.T) { ID: "s1", User: model.SubscriptionUser{ID: "u1", Account: "alice"}, RoomID: "r1", + RoomType: model.RoomTypeChannel, SiteID: "site-a", Roles: []model.Role{model.RoleOwner}, HistorySharedSince: &hss, @@ -412,6 +413,12 @@ func TestRoomTypeValues(t *testing.T) { if model.RoomTypeDM != "dm" { t.Errorf("RoomTypeDM = %q", model.RoomTypeDM) } + if model.RoomTypeBotDM != "botDM" { + t.Errorf("RoomTypeBotDM = %q", model.RoomTypeBotDM) + } + if model.RoomTypeDiscussion != "discussion" { + t.Errorf("RoomTypeDiscussion = %q", model.RoomTypeDiscussion) + } } func TestRoleValues(t *testing.T) { diff --git a/pkg/model/room.go b/pkg/model/room.go index 67d1ee384..78e1284cc 100644 --- a/pkg/model/room.go +++ b/pkg/model/room.go @@ -5,8 +5,10 @@ import "time" type RoomType string const ( - RoomTypeChannel RoomType = "channel" - RoomTypeDM RoomType = "dm" + RoomTypeChannel RoomType = "channel" + RoomTypeDM RoomType = "dm" + RoomTypeBotDM RoomType = "botDM" + RoomTypeDiscussion RoomType = "discussion" ) type Room struct { diff --git a/pkg/model/subscription.go b/pkg/model/subscription.go index 315bfceeb..e13137531 100644 --- a/pkg/model/subscription.go +++ b/pkg/model/subscription.go @@ -24,6 +24,7 @@ type Subscription struct { ID string `json:"id" bson:"_id"` User SubscriptionUser `json:"u" bson:"u"` RoomID string `json:"roomId" bson:"roomId"` + RoomType RoomType `json:"roomType" bson:"roomType"` SiteID string `json:"siteId" bson:"siteId"` Roles []Role `json:"roles" bson:"roles"` HistorySharedSince *time.Time `json:"historySharedSince,omitempty" bson:"historySharedSince,omitempty"` From fce1fb7d4947fe1cc47eb685fdf95bc99e1cd2e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 01:56:27 +0000 Subject: [PATCH 03/14] room-service: populate Subscription.RoomType on CreateRoom The owner subscription auto-created by handleCreateRoom now carries the room type from the create request so downstream consumers can route on the kind without re-fetching the room. --- room-service/handler.go | 1 + room-service/handler_test.go | 3 +++ 2 files changed, 4 insertions(+) diff --git a/room-service/handler.go b/room-service/handler.go index e74136844..4aca6fa10 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -127,6 +127,7 @@ func (h *Handler) handleCreateRoom(ctx context.Context, data []byte) ([]byte, er ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: req.CreatedBy, Account: req.CreatedByAccount}, RoomID: room.ID, + RoomType: req.Type, SiteID: req.SiteID, Roles: []model.Role{model.RoleOwner}, HistorySharedSince: &now, diff --git a/room-service/handler_test.go b/room-service/handler_test.go index 6f2721790..bc5867171 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -47,6 +47,9 @@ func TestHandler_CreateRoom(t *testing.T) { if capturedSub == nil || capturedSub.User.Account != "alice" { t.Errorf("expected owner subscription with Account=alice, got %+v", capturedSub) } + if capturedSub != nil && capturedSub.RoomType != model.RoomTypeChannel { + t.Errorf("expected owner subscription RoomType=%q, got %q", model.RoomTypeChannel, capturedSub.RoomType) + } } func TestHandler_UpdateRole_Success(t *testing.T) { From 40905506c64099045d90d204bc5bf764119918f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 01:59:35 +0000 Subject: [PATCH 04/14] room-worker: populate Subscription.RoomType on add and remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit processAddMembers hardcodes RoomTypeChannel — DM and botDM rooms do not support add-member. processRemoveIndividual and processRemoveOrg each do one GetRoom per operation and stamp room.Type onto the partial Subscription carried by SubscriptionUpdateEvent. The room fetch is non-fatal: a lookup error logs at warn and the event still fires with an empty RoomType, matching the existing best-effort treatment of notification payloads. --- room-worker/handler.go | 29 +++++++++++++++++++++++++---- room-worker/handler_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/room-worker/handler.go b/room-worker/handler.go index 1583af70d..b85cfdc39 100644 --- a/room-worker/handler.go +++ b/room-worker/handler.go @@ -174,12 +174,22 @@ func (h *Handler) processRemoveIndividual(ctx context.Context, req *model.Remove now := time.Now().UTC() + // Fetch the room so the removed-event payload carries RoomType. + // Non-fatal: on failure we proceed with an empty RoomType. + var roomType model.RoomType + if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { + slog.Warn("get room failed during remove-individual", "error", err, "roomID", req.RoomID) + } else if room != nil { + roomType = room.Type + } + // Subscription update event subEvt := model.SubscriptionUpdateEvent{ UserID: user.ID, Subscription: model.Subscription{ - RoomID: req.RoomID, - User: model.SubscriptionUser{ID: user.ID, Account: req.Account}, + RoomID: req.RoomID, + RoomType: roomType, + User: model.SubscriptionUser{ID: user.ID, Account: req.Account}, }, Action: "removed", Timestamp: now.UnixMilli(), @@ -289,6 +299,15 @@ func (h *Handler) processRemoveOrg(ctx context.Context, req *model.RemoveMemberR now := time.Now().UTC() + // Fetch the room once so every per-account removed event carries RoomType. + // Non-fatal: on failure we proceed with an empty RoomType. + var roomType model.RoomType + if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { + slog.Warn("get room failed during remove-org", "error", err, "roomID", req.RoomID) + } else if room != nil { + roomType = room.Type + } + // Publish per-account subscription update and collect cross-site accounts sectName := "" for _, m := range toRemove { @@ -297,8 +316,9 @@ func (h *Handler) processRemoveOrg(ctx context.Context, req *model.RemoveMemberR } subEvt := model.SubscriptionUpdateEvent{ Subscription: model.Subscription{ - RoomID: req.RoomID, - User: model.SubscriptionUser{Account: m.Account}, + RoomID: req.RoomID, + RoomType: roomType, + User: model.SubscriptionUser{Account: m.Account}, }, Action: "removed", Timestamp: now.UnixMilli(), @@ -428,6 +448,7 @@ func (h *Handler) processAddMembers(ctx context.Context, data []byte) error { ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, RoomID: req.RoomID, + RoomType: model.RoomTypeChannel, SiteID: room.SiteID, Roles: []model.Role{model.RoleMember}, JoinedAt: acceptedAt, diff --git a/room-worker/handler_test.go b/room-worker/handler_test.go index bf0c1a071..2b1e53c49 100644 --- a/room-worker/handler_test.go +++ b/room-worker/handler_test.go @@ -310,6 +310,9 @@ func TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, siteID, func(_ context.Context, subj string, data []byte, _ string) error { @@ -335,6 +338,16 @@ func TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly(t *testing.T) { assert.True(t, subjSet[subject.SubscriptionUpdate(account)], "expected subscription update published") assert.True(t, subjSet[subject.MemberEvent(roomID)], "expected member event published") + // Verify the RoomType is carried on the SubscriptionUpdateEvent payload + for _, p := range published { + if p.subj != subject.SubscriptionUpdate(account) { + continue + } + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(p.data, &evt)) + assert.Equal(t, model.RoomTypeChannel, evt.Subscription.RoomType, "subscription update should carry RoomType") + } + // Verify timestamps on all events for _, p := range published { var raw map[string]json.RawMessage @@ -483,6 +496,9 @@ func TestHandler_ProcessRemoveMember_OwnerRemovesIndividual(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, siteID, func(_ context.Context, subj string, data []byte, _ string) error { @@ -533,6 +549,7 @@ func TestHandler_ProcessAddMembers(t *testing.T) { assert.Len(t, subs, 2) for _, s := range subs { assert.Equal(t, "site-a", s.SiteID) + assert.Equal(t, model.RoomTypeChannel, s.RoomType) assert.Equal(t, []model.Role{model.RoleMember}, s.Roles) require.NotNil(t, s.HistorySharedSince) assert.Equal(t, s.JoinedAt, *s.HistorySharedSince) @@ -861,6 +878,9 @@ func TestHandler_ProcessRemoveMember_OwnerRemovesOrg(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) // recount after removal + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, siteID, func(_ context.Context, subj string, data []byte, _ string) error { @@ -919,6 +939,9 @@ func TestHandler_ProcessRemoveMember_CrossSiteOutbox(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, localSite, func(_ context.Context, subj string, data []byte, _ string) error { @@ -1143,6 +1166,9 @@ func TestHandler_ProcessRemoveIndividual_OutboxFailurePropagates(t *testing.T) { Return(int64(1), nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) outboxSubj := subject.Outbox(localSite, userSite, "member_removed") publish := func(_ context.Context, subj string, _ []byte, _ string) error { @@ -1181,6 +1207,9 @@ func TestHandler_ProcessRemoveOrg_OutboxFailurePropagates(t *testing.T) { store.EXPECT().DeleteSubscriptionsByAccounts(gomock.Any(), roomID, []string{"carol"}).Return(int64(1), nil) store.EXPECT().DeleteRoomMember(gomock.Any(), roomID, model.RoomMemberOrg, orgID).Return(nil) store.EXPECT().ReconcileUserCount(gomock.Any(), roomID).Return(nil) + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) outboxSubj := subject.Outbox(localSite, remoteSite, "member_removed") publish := func(_ context.Context, subj string, _ []byte, _ string) error { From e74f6ac2edbd1204ebec4d6eb0e8b87ff3cc0dcb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 02:00:16 +0000 Subject: [PATCH 05/14] inbox-worker: populate RoomTypeChannel on cross-site member_added Cross-site member_added events only originate from rooms that support add-member (channels and discussions). Hardcoding RoomTypeChannel mirrors room-worker's processAddMembers logic so every persisted subscription carries a non-empty RoomType. --- inbox-worker/handler.go | 1 + inbox-worker/handler_test.go | 3 +++ 2 files changed, 4 insertions(+) diff --git a/inbox-worker/handler.go b/inbox-worker/handler.go index 3786e4a8b..3c8214887 100644 --- a/inbox-worker/handler.go +++ b/inbox-worker/handler.go @@ -94,6 +94,7 @@ func (h *Handler) handleMemberAdded(ctx context.Context, evt *model.OutboxEvent) ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, RoomID: event.RoomID, + RoomType: model.RoomTypeChannel, SiteID: event.SiteID, Roles: []model.Role{model.RoleMember}, HistorySharedSince: historySharedSince, diff --git a/inbox-worker/handler_test.go b/inbox-worker/handler_test.go index 211efef77..b06311662 100644 --- a/inbox-worker/handler_test.go +++ b/inbox-worker/handler_test.go @@ -218,6 +218,9 @@ func TestHandleEvent_MemberAdded(t *testing.T) { if len(sub.Roles) != 1 || sub.Roles[0] != model.RoleMember { t.Errorf("subscription Roles = %v, want [%q]", sub.Roles, model.RoleMember) } + if sub.RoomType != model.RoomTypeChannel { + t.Errorf("subscription RoomType = %q, want %q", sub.RoomType, model.RoomTypeChannel) + } if sub.ID == "" { t.Error("subscription ID should be non-empty (generated UUID)") } From 4db0421ae573e108815095e5c0084faeecdd08ef Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 05:11:12 +0000 Subject: [PATCH 06/14] docs: align spec with what shipped on this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes stale claims that this PR removes RoomTypeGroup or renames its call sites — PR #118 already did both. Updates the commit-strategy section to reflect the five commits actually made instead of the single-commit plan that predated the rebase. --- ...6-04-23-roomtype-on-subscription-design.md | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md b/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md index 4bdc7cea1..fca9224a5 100644 --- a/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md +++ b/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md @@ -3,10 +3,11 @@ ## Summary Add a `RoomType` field to `model.Subscription` so that every subscription record -carries the kind of room it belongs to. Align the `RoomType` enum with the -product's current room taxonomy: `dm`, `channel`, `botDM`, `discussion`. -Remove the legacy `RoomTypeGroup = "group"` constant and replace every -call site with `RoomTypeChannel`. +carries the kind of room it belongs to. Add the new `RoomTypeBotDM` and +`RoomTypeDiscussion` constants to align the `RoomType` enum with the product's +current taxonomy: `dm`, `channel`, `botDM`, `discussion`. The legacy +`RoomTypeGroup = "group"` constant was already removed by PR #118 — this change +does not redo that work. ## Motivation @@ -21,13 +22,13 @@ product's current naming. ### In scope -1. `pkg/model/room.go` — enum changes. -2. `pkg/model/subscription.go` — new field. +1. `pkg/model/room.go` — add `RoomTypeBotDM` and `RoomTypeDiscussion` constants. +2. `pkg/model/subscription.go` — new `RoomType` field. 3. All production code that creates a `model.Subscription` — populate the new field. -4. All production code and tests that reference `model.RoomTypeGroup` — rename - to `model.RoomTypeChannel`. -5. Unit and integration tests updated accordingly. +4. The two `room-worker` "removed" event payloads — populate the new field on + the partial `Subscription` literal carried by `SubscriptionUpdateEvent`. +5. Unit tests updated accordingly. ### Out of scope @@ -51,9 +52,9 @@ const ( ) ``` -- Remove `RoomTypeGroup`. - Add `RoomTypeBotDM` and `RoomTypeDiscussion`. -- `RoomTypeChannel` and `RoomTypeDM` are unchanged. +- `RoomTypeChannel` and `RoomTypeDM` are unchanged. `RoomTypeGroup` was already + removed by PR #118. ### 2. `pkg/model/subscription.go` @@ -141,10 +142,15 @@ Following the repository's TDD rules: write failing tests, then implement. ## Commit strategy -A single commit on branch `claude/add-roomtype-subscription-Uqow3` covering -the model change, all call-site updates, renames, and tests. The rename is -atomic — splitting across commits would leave the tree unbuildable between -commits. +Five commits on branch `claude/add-roomtype-subscription-Uqow3`, each leaving +the tree green: + +1. Spec doc. +2. `pkg/model`: add the new constants and the `RoomType` field, plus model tests. +3. `room-service`: populate `RoomType` on the CreateRoom owner subscription. +4. `room-worker`: populate `RoomType` on `processAddMembers`, + `processRemoveIndividual`, and `processRemoveOrg`. +5. `inbox-worker`: populate `RoomType` on the cross-site `member_added` handler. ## Risks From 24725f63d7816d0b4a17393c2a99709f7a840bb1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 05:29:22 +0000 Subject: [PATCH 07/14] docs: scaffold roomtype-on-subscription implementation plan Adds the plan header, file structure table, prerequisites, and the four-task outline. Subsequent commits will fill in each task body one at a time. --- ...026-04-27-roomtype-on-subscription-plan.md | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md diff --git a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md new file mode 100644 index 000000000..1717f63de --- /dev/null +++ b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md @@ -0,0 +1,45 @@ +# RoomType on Subscription Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add a `RoomType` field to `model.Subscription`, add the new `RoomTypeBotDM` and `RoomTypeDiscussion` constants, and populate `RoomType` at every subscription creation site and on the partial Subscription payloads carried by removed `SubscriptionUpdateEvent`s. + +**Architecture:** Denormalise the room kind onto the subscription document so downstream consumers (frontend room-list categorization, notification rules, future per-type handling) can route on `Subscription.RoomType` without an extra room lookup. Each producer either reads the type from a fetched room or hardcodes `RoomTypeChannel` when only that type is reachable. + +**Tech Stack:** Go 1.25, MongoDB driver v2 (`go.mongodb.org/mongo-driver/v2`), `go.uber.org/mock` (mockgen), `stretchr/testify` (assertions), NATS JetStream consumers via `pkg/idgen` and `pkg/subject`. + +**Spec:** `docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md`. + +--- + +## Prerequisites + +- Branch off `origin/main` after PR #118 ("Remove-member / role-update hardening") has merged. PR #118 already removed `RoomTypeGroup`, deleted `processInvite`, renamed `broadcast-worker`'s `publishGroupEvent` → `publishChannelEvent`, and updated `room-service`'s role-update guard. Do not redo any of that. +- All commands assume the repo root `/home/user/chat`. Use the `Makefile` targets — never raw `go` commands. +- Pre-commit hook runs lint + tests; commits fail if either fails. + +## File Structure + +| File | Responsibility | Touched in | +|---|---|---| +| `pkg/model/room.go` | Add new RoomType constants. | Task 1 | +| `pkg/model/subscription.go` | Add `RoomType` field on `Subscription`. | Task 1 | +| `pkg/model/model_test.go` | Assert new constants; round-trip the new field. | Task 1 | +| `room-service/handler.go` | Stamp `req.Type` onto the auto-created owner sub. | Task 2 | +| `room-service/handler_test.go` | Assert captured sub carries `RoomType`. | Task 2 | +| `room-worker/handler.go` | Hardcode `RoomTypeChannel` on `processAddMembers` subs; fetch room and stamp `RoomType` on the partial subs in `processRemoveIndividual` and `processRemoveOrg` event payloads. | Task 3 | +| `room-worker/handler_test.go` | Add `GetRoom` mocks + `RoomType` assertions on five existing tests. | Task 3 | +| `inbox-worker/handler.go` | Hardcode `RoomTypeChannel` on cross-site `member_added` sub. | Task 4 | +| `inbox-worker/handler_test.go` | Assert created sub has `RoomType: RoomTypeChannel`. | Task 4 | + +No store interfaces change. `room-worker`'s `SubscriptionStore` already has `GetRoom`; no `make generate` needed. + +## Tasks + +1. **`pkg/model` foundation** — new constants + new `Subscription.RoomType` field + model tests. Backward-compatible: existing call sites keep compiling because the new field defaults to `""`. +2. **`room-service` CreateRoom** — owner subscription carries `req.Type`. +3. **`room-worker`** — three sub literals updated: + - `processAddMembers`: hardcode `RoomTypeChannel`. + - `processRemoveIndividual`: fetch the room, stamp `RoomType` on the partial sub literal carried by the "removed" `SubscriptionUpdateEvent`. + - `processRemoveOrg`: same, with one room fetch reused across the per-account event loop. +4. **`inbox-worker`** — cross-site `handleMemberAdded` hardcodes `RoomTypeChannel` (only channel/discussion-style rooms ever produce cross-site `member_added` events, never DM/botDM). From 1ec0a80b1cb5947a2cbcff991c6f0a1ed57775ee Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 05:29:55 +0000 Subject: [PATCH 08/14] docs: plan task 1 (pkg/model foundation) Adds the bite-sized TDD steps for introducing RoomTypeBotDM and RoomTypeDiscussion constants and the RoomType field on Subscription. --- ...026-04-27-roomtype-on-subscription-plan.md | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md index 1717f63de..9f2fa91b0 100644 --- a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md +++ b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md @@ -43,3 +43,131 @@ No store interfaces change. `room-worker`'s `SubscriptionStore` already has `Get - `processRemoveIndividual`: fetch the room, stamp `RoomType` on the partial sub literal carried by the "removed" `SubscriptionUpdateEvent`. - `processRemoveOrg`: same, with one room fetch reused across the per-account event loop. 4. **`inbox-worker`** — cross-site `handleMemberAdded` hardcodes `RoomTypeChannel` (only channel/discussion-style rooms ever produce cross-site `member_added` events, never DM/botDM). + +--- + +### Task 1: `pkg/model` foundation + +**Files:** +- Modify: `pkg/model/room.go` (the `RoomType` const block) +- Modify: `pkg/model/subscription.go` (the `Subscription` struct) +- Modify: `pkg/model/model_test.go` (`TestSubscriptionJSON`, `TestRoomTypeValues`) + +This task is independently buildable: every existing call site keeps compiling because the new `Subscription.RoomType` field defaults to `""`. + +- [ ] **Step 1.1: Update `TestSubscriptionJSON` to set the new field** + + Edit `pkg/model/model_test.go`. Find the `TestSubscriptionJSON` function and add `RoomType: model.RoomTypeChannel` between `RoomID` and `SiteID` so the round-trip exercises the new field: + + ```go + func TestSubscriptionJSON(t *testing.T) { + hss := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + s := model.Subscription{ + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleOwner}, + HistorySharedSince: &hss, + JoinedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), + LastSeenAt: time.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC), + HasMention: true, + } + // (rest of the test unchanged — it already round-trips s through JSON) + } + ``` + +- [ ] **Step 1.2: Update `TestRoomTypeValues` to assert the four constants** + + Replace the existing `TestRoomTypeValues` body with: + + ```go + func TestRoomTypeValues(t *testing.T) { + if model.RoomTypeChannel != "channel" { + t.Errorf("RoomTypeChannel = %q", model.RoomTypeChannel) + } + if model.RoomTypeDM != "dm" { + t.Errorf("RoomTypeDM = %q", model.RoomTypeDM) + } + if model.RoomTypeBotDM != "botDM" { + t.Errorf("RoomTypeBotDM = %q", model.RoomTypeBotDM) + } + if model.RoomTypeDiscussion != "discussion" { + t.Errorf("RoomTypeDiscussion = %q", model.RoomTypeDiscussion) + } + } + ``` + +- [ ] **Step 1.3: Run the tests — expect a build failure** + + ``` + make test SERVICE=pkg/model + ``` + + Expected output (verbatim): + ``` + pkg/model/model_test.go:387:3: unknown field RoomType in struct literal of type model.Subscription + pkg/model/model_test.go:416:11: undefined: model.RoomTypeBotDM + pkg/model/model_test.go:419:11: undefined: model.RoomTypeDiscussion + FAIL github.com/hmchangw/chat/pkg/model [build failed] + ``` + +- [ ] **Step 1.4: Add the new constants in `pkg/model/room.go`** + + Replace the existing `const ( ... )` block with: + + ```go + const ( + RoomTypeChannel RoomType = "channel" + RoomTypeDM RoomType = "dm" + RoomTypeBotDM RoomType = "botDM" + RoomTypeDiscussion RoomType = "discussion" + ) + ``` + +- [ ] **Step 1.5: Add the `RoomType` field on `Subscription`** + + Edit `pkg/model/subscription.go`. Insert the new field between `RoomID` and `SiteID`: + + ```go + type Subscription struct { + ID string `json:"id" bson:"_id"` + User SubscriptionUser `json:"u" bson:"u"` + RoomID string `json:"roomId" bson:"roomId"` + RoomType RoomType `json:"roomType" bson:"roomType"` + SiteID string `json:"siteId" bson:"siteId"` + Roles []Role `json:"roles" bson:"roles"` + HistorySharedSince *time.Time `json:"historySharedSince,omitempty" bson:"historySharedSince,omitempty"` + JoinedAt time.Time `json:"joinedAt" bson:"joinedAt"` + LastSeenAt time.Time `json:"lastSeenAt" bson:"lastSeenAt"` + HasMention bool `json:"hasMention" bson:"hasMention"` + } + ``` + +- [ ] **Step 1.6: Run the tests — expect green** + + ``` + make test SERVICE=pkg/model + ``` + + Expected: + ``` + ok github.com/hmchangw/chat/pkg/model 1.0xxs + ok github.com/hmchangw/chat/pkg/model/cassandra (cached) + ``` + +- [ ] **Step 1.7: Lint clean** + + ``` + make lint + ``` + + Expected: `0 issues.` + +- [ ] **Step 1.8: Commit** + + ```bash + git add pkg/model/ + git commit -m "model: add RoomType on Subscription and botDM/discussion constants" + ``` From 6b87ebb133ea88e0f1cf106006dbcb849f1cd62b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 05:30:21 +0000 Subject: [PATCH 09/14] docs: plan task 2 (room-service CreateRoom) Adds the TDD steps for stamping req.Type onto the auto-created owner subscription returned by handleCreateRoom. --- ...026-04-27-roomtype-on-subscription-plan.md | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md index 9f2fa91b0..170ec7020 100644 --- a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md +++ b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md @@ -171,3 +171,78 @@ This task is independently buildable: every existing call site keeps compiling b git add pkg/model/ git commit -m "model: add RoomType on Subscription and botDM/discussion constants" ``` + +--- + +### Task 2: `room-service` CreateRoom owner subscription + +**Files:** +- Modify: `room-service/handler.go` (the `handleCreateRoom` function — the `sub := model.Subscription{...}` literal) +- Modify: `room-service/handler_test.go` (`TestHandler_CreateRoom`) + +Depends on Task 1 (`Subscription.RoomType` must exist). + +- [ ] **Step 2.1: Add the RoomType assertion to `TestHandler_CreateRoom`** + + Edit `room-service/handler_test.go`. The test already captures the auto-created subscription via `var capturedSub *model.Subscription`. Append a `RoomType` assertion at the bottom of the test, just before the closing brace: + + ```go + if capturedSub != nil && capturedSub.RoomType != model.RoomTypeChannel { + t.Errorf("expected owner subscription RoomType=%q, got %q", model.RoomTypeChannel, capturedSub.RoomType) + } + ``` + + The request fixture in this test already uses `Type: model.RoomTypeChannel`, so the assertion is internally consistent. + +- [ ] **Step 2.2: Run the test — expect red** + + ``` + make test SERVICE=room-service + ``` + + Expected: + ``` + --- FAIL: TestHandler_CreateRoom (0.00s) + handler_test.go:51: expected owner subscription RoomType="channel", got "" + ``` + +- [ ] **Step 2.3: Stamp `req.Type` onto the owner subscription** + + Edit `room-service/handler.go` inside `handleCreateRoom`. The block `sub := model.Subscription{...}` builds the auto-created owner sub. Add `RoomType: req.Type,` between `RoomID` and `SiteID`: + + ```go + // Auto-create owner subscription + sub := model.Subscription{ + ID: idgen.GenerateID(), + User: model.SubscriptionUser{ID: req.CreatedBy, Account: req.CreatedByAccount}, + RoomID: room.ID, + RoomType: req.Type, + SiteID: req.SiteID, + Roles: []model.Role{model.RoleOwner}, + HistorySharedSince: &now, + JoinedAt: now, + } + ``` + +- [ ] **Step 2.4: Run the test — expect green** + + ``` + make test SERVICE=room-service + ``` + + Expected: `ok github.com/hmchangw/chat/room-service 1.0xxs` + +- [ ] **Step 2.5: Lint clean** + + ``` + make lint + ``` + + Expected: `0 issues.` + +- [ ] **Step 2.6: Commit** + + ```bash + git add room-service/ + git commit -m "room-service: populate Subscription.RoomType on CreateRoom" + ``` From 9a4a41cc9c1ec25856ec2f463d151c7bcc24608d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 05:31:24 +0000 Subject: [PATCH 10/14] docs: plan task 3 (room-worker add and remove paths) Adds the TDD steps for hardcoding RoomTypeChannel on processAddMembers and for fetching the room (non-fatal) inside processRemoveIndividual and processRemoveOrg so the partial Subscription on each removed event carries RoomType. Covers all five existing tests that need GetRoom mocks plus the two outbox-failure tests that hit the same publish path. --- ...026-04-27-roomtype-on-subscription-plan.md | 224 ++++++++++++++++++ 1 file changed, 224 insertions(+) diff --git a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md index 170ec7020..3a2b868a9 100644 --- a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md +++ b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md @@ -246,3 +246,227 @@ Depends on Task 1 (`Subscription.RoomType` must exist). git add room-service/ git commit -m "room-service: populate Subscription.RoomType on CreateRoom" ``` + +--- + +### Task 3: `room-worker` — add and remove paths + +**Files:** +- Modify: `room-worker/handler.go` + - `processAddMembers` — `sub := &model.Subscription{...}` literal inside the per-account loop + - `processRemoveIndividual` — the partial `model.Subscription{...}` literal inside the `SubscriptionUpdateEvent` + - `processRemoveOrg` — the partial `model.Subscription{...}` literal inside the per-account event loop +- Modify: `room-worker/handler_test.go` + - `TestHandler_ProcessAddMembers` + - `TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly` + - `TestHandler_ProcessRemoveMember_OwnerRemovesIndividual` + - `TestHandler_ProcessRemoveMember_OwnerRemovesOrg` + - `TestHandler_ProcessRemoveMember_CrossSiteOutbox` + - `TestHandler_ProcessRemoveIndividual_OutboxFailurePropagates` + - `TestHandler_ProcessRemoveOrg_OutboxFailurePropagates` + +Depends on Task 1. The room-worker `SubscriptionStore` interface already has `GetRoom`; no `make generate` needed. + +**Design notes (from spec §4):** + +The remove paths fetch the room once per operation and stamp `room.Type` onto the partial Subscription literal that the `SubscriptionUpdateEvent` payload carries. The fetch is **non-fatal** — on `GetRoom` error, log at warn and continue with `RoomType: ""`. The removal itself must not block on this lookup. + +`processAddMembers` does NOT do a room-fetch — it hardcodes `RoomTypeChannel` because DM/botDM rooms reject `member.add` upstream. + +- [ ] **Step 3.1: Update `TestHandler_ProcessAddMembers` — assert RoomType on every created sub** + + Inside the existing `BulkCreateSubscriptions` `DoAndReturn` callback, add a per-sub assertion: + + ```go + store.EXPECT().BulkCreateSubscriptions(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, subs []*model.Subscription) error { + assert.Len(t, subs, 2) + for _, s := range subs { + assert.Equal(t, "site-a", s.SiteID) + assert.Equal(t, model.RoomTypeChannel, s.RoomType) + assert.Equal(t, []model.Role{model.RoleMember}, s.Roles) + require.NotNil(t, s.HistorySharedSince) + assert.Equal(t, s.JoinedAt, *s.HistorySharedSince) + } + return nil + }) + ``` + +- [ ] **Step 3.2: Update `TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly` — add `GetRoom` mock and `RoomType` assertion** + + Add a new `EXPECT` for `GetRoom` after the existing `ReconcileUserCount` expect: + + ```go + store.EXPECT(). + ReconcileUserCount(gomock.Any(), roomID).Return(nil) + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) + ``` + + Then after the `assert.True(t, subjSet[subject.MemberEvent(roomID)], ...)` line, add: + + ```go + // The subscription update event should carry the RoomType from the fetched room. + for _, p := range published { + if p.subj != subject.SubscriptionUpdate(account) { + continue + } + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(p.data, &evt)) + assert.Equal(t, model.RoomTypeChannel, evt.Subscription.RoomType, "subscription update should carry RoomType") + } + ``` + +- [ ] **Step 3.3: Update `TestHandler_ProcessRemoveMember_OwnerRemovesIndividual` — add `GetRoom` mock** + + After the existing `ReconcileUserCount` expect, append: + + ```go + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) + ``` + + No additional assertion needed — the previous test already asserts the RoomType payload contract. + +- [ ] **Step 3.4: Update `TestHandler_ProcessRemoveMember_OwnerRemovesOrg` — add `GetRoom` mock** + + After the existing `ReconcileUserCount(... )` expect, append: + + ```go + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) + ``` + +- [ ] **Step 3.5: Update `TestHandler_ProcessRemoveMember_CrossSiteOutbox` — add `GetRoom` mock** + + After the existing `ReconcileUserCount(... )` expect, append: + + ```go + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) + ``` + +- [ ] **Step 3.6: Update both `OutboxFailurePropagates` tests — add `GetRoom` mock** + + Both `TestHandler_ProcessRemoveIndividual_OutboxFailurePropagates` and `TestHandler_ProcessRemoveOrg_OutboxFailurePropagates` exercise the publish path, so they will also hit the new `GetRoom` call. Add this expect after `ReconcileUserCount` in each test: + + ```go + store.EXPECT(). + GetRoom(gomock.Any(), roomID). + Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) + ``` + +- [ ] **Step 3.7: Run tests — expect red** + + ``` + make test SERVICE=room-worker + ``` + + Expected failures: `TestHandler_ProcessAddMembers` (`RoomType = ""`), and the four+two remove tests fail with `missing call(s) to *main.MockSubscriptionStore.GetRoom`. + +- [ ] **Step 3.8: Hardcode `RoomTypeChannel` on `processAddMembers` subs** + + In `room-worker/handler.go` find the literal inside `processAddMembers`: + + ```go + sub := &model.Subscription{ + ID: idgen.GenerateID(), + User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, + RoomID: req.RoomID, + RoomType: model.RoomTypeChannel, + SiteID: room.SiteID, + Roles: []model.Role{model.RoleMember}, + JoinedAt: acceptedAt, + } + ``` + +- [ ] **Step 3.9: Fetch room and stamp RoomType in `processRemoveIndividual`** + + In `room-worker/handler.go` find the block that begins with `now := time.Now().UTC()` followed by `// Subscription update event` (inside `processRemoveIndividual`). Insert a `GetRoom` call between them and use the fetched type when building the partial Subscription: + + ```go + now := time.Now().UTC() + + // Fetch the room so the removed-event payload carries RoomType. + // Non-fatal: on failure we proceed with an empty RoomType. + var roomType model.RoomType + if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { + slog.Warn("get room failed during remove-individual", "error", err, "roomID", req.RoomID) + } else if room != nil { + roomType = room.Type + } + + // Subscription update event + subEvt := model.SubscriptionUpdateEvent{ + UserID: user.ID, + Subscription: model.Subscription{ + RoomID: req.RoomID, + RoomType: roomType, + User: model.SubscriptionUser{ID: user.ID, Account: req.Account}, + }, + Action: "removed", + Timestamp: now.UnixMilli(), + } + ``` + +- [ ] **Step 3.10: Fetch room and stamp RoomType in `processRemoveOrg`** + + In `room-worker/handler.go` find the matching `now := time.Now().UTC()` followed by `// Publish per-account subscription update and collect cross-site accounts` block (inside `processRemoveOrg`). Make the same insertion — one `GetRoom` call before the loop, reuse `roomType` per iteration: + + ```go + now := time.Now().UTC() + + // Fetch the room once so every per-account removed event carries RoomType. + // Non-fatal: on failure we proceed with an empty RoomType. + var roomType model.RoomType + if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { + slog.Warn("get room failed during remove-org", "error", err, "roomID", req.RoomID) + } else if room != nil { + roomType = room.Type + } + + // Publish per-account subscription update and collect cross-site accounts + sectName := "" + for _, m := range toRemove { + if m.SectName != "" { + sectName = m.SectName + } + subEvt := model.SubscriptionUpdateEvent{ + Subscription: model.Subscription{ + RoomID: req.RoomID, + RoomType: roomType, + User: model.SubscriptionUser{Account: m.Account}, + }, + Action: "removed", + Timestamp: now.UnixMilli(), + } + // ...rest of the loop unchanged + } + ``` + +- [ ] **Step 3.11: Run tests — expect green** + + ``` + make test SERVICE=room-worker + ``` + + Expected: `ok github.com/hmchangw/chat/room-worker 1.0xxs` + +- [ ] **Step 3.12: Lint clean** + + ``` + make lint + ``` + + Expected: `0 issues.` + +- [ ] **Step 3.13: Commit** + + ```bash + git add room-worker/ + git commit -m "room-worker: populate Subscription.RoomType on add and remove" + ``` From 31b5da784c9dd5ad92a4caf975390b2dfab53121 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 05:32:20 +0000 Subject: [PATCH 11/14] docs: plan task 4 (inbox-worker) + final verification appendix Adds the TDD steps for hardcoding RoomTypeChannel on the cross-site member_added handler and a final verification + push checklist covering full make test, make lint, integration tests, and the push command (with --force-with-lease guidance for branches that already have a prior tip on the remote). Also includes a Risks and Rollback section. --- ...026-04-27-roomtype-on-subscription-plan.md | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md index 3a2b868a9..535b05263 100644 --- a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md +++ b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md @@ -470,3 +470,127 @@ The remove paths fetch the room once per operation and stamp `room.Type` onto th git add room-worker/ git commit -m "room-worker: populate Subscription.RoomType on add and remove" ``` + +--- + +### Task 4: `inbox-worker` cross-site member_added + +**Files:** +- Modify: `inbox-worker/handler.go` (the `sub := &model.Subscription{...}` literal inside `handleMemberAdded`) +- Modify: `inbox-worker/handler_test.go` (`TestHandleEvent_MemberAdded`) + +Depends on Task 1. + +Cross-site `member_added` events only fire for rooms that support add-member (channels and discussions). Hardcoding `RoomTypeChannel` mirrors `room-worker.processAddMembers` so every persisted subscription carries a non-empty `RoomType`. + +- [ ] **Step 4.1: Add the RoomType assertion to `TestHandleEvent_MemberAdded`** + + In `inbox-worker/handler_test.go`, find the per-field assertion block on the captured `sub`. After the `Roles` assertion and before the `sub.ID == ""` check, add: + + ```go + if sub.RoomType != model.RoomTypeChannel { + t.Errorf("subscription RoomType = %q, want %q", sub.RoomType, model.RoomTypeChannel) + } + ``` + +- [ ] **Step 4.2: Run tests — expect red** + + ``` + make test SERVICE=inbox-worker + ``` + + Expected: + ``` + --- FAIL: TestHandleEvent_MemberAdded (0.00s) + handler_test.go:222: subscription RoomType = "", want "channel" + ``` + +- [ ] **Step 4.3: Hardcode `RoomTypeChannel` on the cross-site sub** + + In `inbox-worker/handler.go` find the literal inside `handleMemberAdded`. Add `RoomType: model.RoomTypeChannel,` between `RoomID` and `SiteID`: + + ```go + sub := &model.Subscription{ + ID: idgen.GenerateID(), + User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, + RoomID: event.RoomID, + RoomType: model.RoomTypeChannel, + SiteID: event.SiteID, + Roles: []model.Role{model.RoleMember}, + HistorySharedSince: historySharedSince, + JoinedAt: joinedAt, + } + ``` + +- [ ] **Step 4.4: Run tests — expect green** + + ``` + make test SERVICE=inbox-worker + ``` + + Expected: `ok github.com/hmchangw/chat/inbox-worker 1.0xxs` + +- [ ] **Step 4.5: Lint clean** + + ``` + make lint + ``` + + Expected: `0 issues.` + +- [ ] **Step 4.6: Commit** + + ```bash + git add inbox-worker/ + git commit -m "inbox-worker: populate RoomTypeChannel on cross-site member_added" + ``` + +--- + +## Final verification + +After all four tasks land, sweep the whole repo and push: + +- [ ] **V.1: Full test sweep** + + ``` + make test + ``` + + Expected: every package shows `ok ...` (no `FAIL`). The `pkg/cassutil`, `pkg/oidc`, `pkg/otelutil`, `pkg/userstore`, etc. lines may show `[no test files]` — that's normal. + +- [ ] **V.2: Full lint sweep** + + ``` + make lint + ``` + + Expected: `0 issues.` + +- [ ] **V.3: Integration tests (where Docker is available)** + + ``` + make test-integration SERVICE=room-worker + make test-integration SERVICE=room-service + make test-integration SERVICE=inbox-worker + ``` + + These require Docker for testcontainers. Skip if not available locally — CI will exercise them. + +- [ ] **V.4: Push to the feature branch** + + ``` + git push -u origin claude/add-roomtype-subscription-Uqow3 + ``` + + If the branch already has the previous (pre-rebase) commits on the remote, force-push with lease: + ``` + git push --force-with-lease origin claude/add-roomtype-subscription-Uqow3 + ``` + +## Risks and rollback + +- **Old subscriptions without `roomType`.** Returned as `RoomType: ""`. No code in this branch reads the field, so old documents are harmless until a future consumer relies on it. A backfill job is out of scope. +- **`GetRoom` failure during remove paths.** Logged at warn; the removal itself still succeeds and the SubscriptionUpdateEvent fires with an empty `RoomType`. Matches the pre-existing best-effort treatment of notification payloads. +- **Future creation site forgets to set `RoomType`.** Mitigated by per-site test assertions in Tasks 2-4. +- **Rollback:** revert the four implementation commits in reverse order. Reverting only the model commit (Task 1) would break compilation of the call sites that use the new field — revert all four together if needed. From 45d2a0a0e10c1671d09b179a450272c112334c0d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 06:22:53 +0000 Subject: [PATCH 12/14] room-worker: extract lookupRoomType + tighten comments Pulls the duplicated GetRoom-then-warn pattern out of processRemoveIndividual and processRemoveOrg into a Handler.lookupRoomType helper that returns "" on lookup failure. Adds brief WHY notes at the two RoomTypeChannel hardcodes (room-worker processAddMembers and inbox-worker handleMemberAdded) so future readers see the upstream invariant that prevents non-channel rooms from reaching those paths. Drops a narrative test comment that the following assertion already conveys. --- inbox-worker/handler.go | 3 +++ room-worker/handler.go | 38 +++++++++++++++++++------------------ room-worker/handler_test.go | 1 - 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/inbox-worker/handler.go b/inbox-worker/handler.go index 3c8214887..bfac84149 100644 --- a/inbox-worker/handler.go +++ b/inbox-worker/handler.go @@ -90,6 +90,9 @@ func (h *Handler) handleMemberAdded(ctx context.Context, evt *model.OutboxEvent) slog.Warn("user not found for account", "account", account) continue } + // RoomType is fixed to channel: cross-site member_added events only + // originate from rooms that support add-member (channel/discussion), + // never from DM/botDM. sub := &model.Subscription{ ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, diff --git a/room-worker/handler.go b/room-worker/handler.go index b85cfdc39..11585782f 100644 --- a/room-worker/handler.go +++ b/room-worker/handler.go @@ -173,15 +173,7 @@ func (h *Handler) processRemoveIndividual(ctx context.Context, req *model.Remove } now := time.Now().UTC() - - // Fetch the room so the removed-event payload carries RoomType. - // Non-fatal: on failure we proceed with an empty RoomType. - var roomType model.RoomType - if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { - slog.Warn("get room failed during remove-individual", "error", err, "roomID", req.RoomID) - } else if room != nil { - roomType = room.Type - } + roomType := h.lookupRoomType(ctx, req.RoomID, "remove-individual") // Subscription update event subEvt := model.SubscriptionUpdateEvent{ @@ -298,15 +290,8 @@ func (h *Handler) processRemoveOrg(ctx context.Context, req *model.RemoveMemberR } now := time.Now().UTC() - - // Fetch the room once so every per-account removed event carries RoomType. - // Non-fatal: on failure we proceed with an empty RoomType. - var roomType model.RoomType - if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { - slog.Warn("get room failed during remove-org", "error", err, "roomID", req.RoomID) - } else if room != nil { - roomType = room.Type - } + // Fetched once: reused across the per-account event loop below. + roomType := h.lookupRoomType(ctx, req.RoomID, "remove-org") // Publish per-account subscription update and collect cross-site accounts sectName := "" @@ -444,6 +429,8 @@ func (h *Handler) processAddMembers(ctx context.Context, data []byte) error { slog.Warn("user not found for account", "account", account) continue } + // RoomType is fixed to channel: room-service rejects member.add for + // any other room kind, so this code path only sees channel rooms. sub := &model.Subscription{ ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, @@ -681,3 +668,18 @@ func mustMarshal(v any) []byte { data, _ := json.Marshal(v) return data } + +// lookupRoomType fetches the room and returns its Type. On failure it logs at +// warn and returns "" — the field is best-effort metadata on notification +// payloads, never load-bearing. +func (h *Handler) lookupRoomType(ctx context.Context, roomID, op string) model.RoomType { + room, err := h.store.GetRoom(ctx, roomID) + if err != nil { + slog.Warn("get room failed", "error", err, "roomID", roomID, "op", op) + return "" + } + if room == nil { + return "" + } + return room.Type +} diff --git a/room-worker/handler_test.go b/room-worker/handler_test.go index 2b1e53c49..353f03385 100644 --- a/room-worker/handler_test.go +++ b/room-worker/handler_test.go @@ -338,7 +338,6 @@ func TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly(t *testing.T) { assert.True(t, subjSet[subject.SubscriptionUpdate(account)], "expected subscription update published") assert.True(t, subjSet[subject.MemberEvent(roomID)], "expected member event published") - // Verify the RoomType is carried on the SubscriptionUpdateEvent payload for _, p := range published { if p.subj != subject.SubscriptionUpdate(account) { continue From c55c05f6dea6709888bdc4df5a7e24ccdfe81f84 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 03:23:33 +0000 Subject: [PATCH 13/14] room-worker: hardcode RoomTypeChannel on remove paths (PR #137 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per mliu33's review on PR #137: member-management ops (member.add, member.remove, member.role-update) only apply to channel rooms — room-service rejects them for any other kind before they reach the room-worker stream. So the runtime GetRoom round-trip in processRemoveIndividual and processRemoveOrg is unnecessary; both sites now hardcode RoomTypeChannel on the partial Subscription literal, matching processAddMembers. The lookupRoomType helper is gone. Tests drop the GetRoom mocks (six call sites) and keep the existing RoomType assertion on the published SubscriptionUpdateEvent payload, which now passes by virtue of the hardcode. Resolves: hmchangw/chat#137 review threads on lines 183, 305, 675. --- room-worker/handler.go | 27 +++++---------------------- room-worker/handler_test.go | 18 ------------------ 2 files changed, 5 insertions(+), 40 deletions(-) diff --git a/room-worker/handler.go b/room-worker/handler.go index 11585782f..44a9ac5a7 100644 --- a/room-worker/handler.go +++ b/room-worker/handler.go @@ -173,14 +173,14 @@ func (h *Handler) processRemoveIndividual(ctx context.Context, req *model.Remove } now := time.Now().UTC() - roomType := h.lookupRoomType(ctx, req.RoomID, "remove-individual") - // Subscription update event + // Subscription update event. RoomType is fixed to channel: room-service + // rejects member.remove for any other room kind. subEvt := model.SubscriptionUpdateEvent{ UserID: user.ID, Subscription: model.Subscription{ RoomID: req.RoomID, - RoomType: roomType, + RoomType: model.RoomTypeChannel, User: model.SubscriptionUser{ID: user.ID, Account: req.Account}, }, Action: "removed", @@ -290,8 +290,6 @@ func (h *Handler) processRemoveOrg(ctx context.Context, req *model.RemoveMemberR } now := time.Now().UTC() - // Fetched once: reused across the per-account event loop below. - roomType := h.lookupRoomType(ctx, req.RoomID, "remove-org") // Publish per-account subscription update and collect cross-site accounts sectName := "" @@ -302,7 +300,7 @@ func (h *Handler) processRemoveOrg(ctx context.Context, req *model.RemoveMemberR subEvt := model.SubscriptionUpdateEvent{ Subscription: model.Subscription{ RoomID: req.RoomID, - RoomType: roomType, + RoomType: model.RoomTypeChannel, User: model.SubscriptionUser{Account: m.Account}, }, Action: "removed", @@ -430,7 +428,7 @@ func (h *Handler) processAddMembers(ctx context.Context, data []byte) error { continue } // RoomType is fixed to channel: room-service rejects member.add for - // any other room kind, so this code path only sees channel rooms. + // any other room kind. sub := &model.Subscription{ ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, @@ -668,18 +666,3 @@ func mustMarshal(v any) []byte { data, _ := json.Marshal(v) return data } - -// lookupRoomType fetches the room and returns its Type. On failure it logs at -// warn and returns "" — the field is best-effort metadata on notification -// payloads, never load-bearing. -func (h *Handler) lookupRoomType(ctx context.Context, roomID, op string) model.RoomType { - room, err := h.store.GetRoom(ctx, roomID) - if err != nil { - slog.Warn("get room failed", "error", err, "roomID", roomID, "op", op) - return "" - } - if room == nil { - return "" - } - return room.Type -} diff --git a/room-worker/handler_test.go b/room-worker/handler_test.go index 353f03385..4e86459a0 100644 --- a/room-worker/handler_test.go +++ b/room-worker/handler_test.go @@ -310,9 +310,6 @@ func TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, siteID, func(_ context.Context, subj string, data []byte, _ string) error { @@ -495,9 +492,6 @@ func TestHandler_ProcessRemoveMember_OwnerRemovesIndividual(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, siteID, func(_ context.Context, subj string, data []byte, _ string) error { @@ -877,9 +871,6 @@ func TestHandler_ProcessRemoveMember_OwnerRemovesOrg(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) // recount after removal - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, siteID, func(_ context.Context, subj string, data []byte, _ string) error { @@ -938,9 +929,6 @@ func TestHandler_ProcessRemoveMember_CrossSiteOutbox(t *testing.T) { Return(nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) var published []publishedMsg h := NewHandler(store, localSite, func(_ context.Context, subj string, data []byte, _ string) error { @@ -1165,9 +1153,6 @@ func TestHandler_ProcessRemoveIndividual_OutboxFailurePropagates(t *testing.T) { Return(int64(1), nil) store.EXPECT(). ReconcileUserCount(gomock.Any(), roomID).Return(nil) - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) outboxSubj := subject.Outbox(localSite, userSite, "member_removed") publish := func(_ context.Context, subj string, _ []byte, _ string) error { @@ -1206,9 +1191,6 @@ func TestHandler_ProcessRemoveOrg_OutboxFailurePropagates(t *testing.T) { store.EXPECT().DeleteSubscriptionsByAccounts(gomock.Any(), roomID, []string{"carol"}).Return(int64(1), nil) store.EXPECT().DeleteRoomMember(gomock.Any(), roomID, model.RoomMemberOrg, orgID).Return(nil) store.EXPECT().ReconcileUserCount(gomock.Any(), roomID).Return(nil) - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) outboxSubj := subject.Outbox(localSite, remoteSite, "member_removed") publish := func(_ context.Context, subj string, _ []byte, _ string) error { From 0840ce6ffb8363c242a625d9ffaa820425363bd6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 03:23:40 +0000 Subject: [PATCH 14/14] docs: align spec and plan with hardcoded RoomType (PR #137 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec §4 and plan Task 3 now describe hardcoding RoomTypeChannel at the two remove sites instead of the previous fetch-room approach, matching the room-worker code change in the prior commit. Also addresses CodeRabbit's MD040 nit by adding language tags (bash for shell commands, text for sample test output) to every previously unlabeled fenced code block in the plan. --- ...026-04-27-roomtype-on-subscription-plan.md | 209 ++++++------------ ...6-04-23-roomtype-on-subscription-design.md | 28 +-- 2 files changed, 79 insertions(+), 158 deletions(-) diff --git a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md index 535b05263..697bc50c2 100644 --- a/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md +++ b/docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md @@ -4,7 +4,7 @@ **Goal:** Add a `RoomType` field to `model.Subscription`, add the new `RoomTypeBotDM` and `RoomTypeDiscussion` constants, and populate `RoomType` at every subscription creation site and on the partial Subscription payloads carried by removed `SubscriptionUpdateEvent`s. -**Architecture:** Denormalise the room kind onto the subscription document so downstream consumers (frontend room-list categorization, notification rules, future per-type handling) can route on `Subscription.RoomType` without an extra room lookup. Each producer either reads the type from a fetched room or hardcodes `RoomTypeChannel` when only that type is reachable. +**Architecture:** Denormalise the room kind onto the subscription document so downstream consumers (frontend room-list categorization, notification rules, future per-type handling) can route on `Subscription.RoomType` without an extra room lookup. Producers either read the type from the room (CreateRoom uses `req.Type`) or hardcode `RoomTypeChannel` for paths that are channel-only by upstream invariant — member-management ops (add/remove/role-update) and cross-site `member_added` events. **Tech Stack:** Go 1.25, MongoDB driver v2 (`go.mongodb.org/mongo-driver/v2`), `go.uber.org/mock` (mockgen), `stretchr/testify` (assertions), NATS JetStream consumers via `pkg/idgen` and `pkg/subject`. @@ -27,8 +27,8 @@ | `pkg/model/model_test.go` | Assert new constants; round-trip the new field. | Task 1 | | `room-service/handler.go` | Stamp `req.Type` onto the auto-created owner sub. | Task 2 | | `room-service/handler_test.go` | Assert captured sub carries `RoomType`. | Task 2 | -| `room-worker/handler.go` | Hardcode `RoomTypeChannel` on `processAddMembers` subs; fetch room and stamp `RoomType` on the partial subs in `processRemoveIndividual` and `processRemoveOrg` event payloads. | Task 3 | -| `room-worker/handler_test.go` | Add `GetRoom` mocks + `RoomType` assertions on five existing tests. | Task 3 | +| `room-worker/handler.go` | Hardcode `RoomTypeChannel` on `processAddMembers`, `processRemoveIndividual`, and `processRemoveOrg` Subscription literals. | Task 3 | +| `room-worker/handler_test.go` | Add `RoomType` assertions on `TestHandler_ProcessAddMembers` and `TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly`. | Task 3 | | `inbox-worker/handler.go` | Hardcode `RoomTypeChannel` on cross-site `member_added` sub. | Task 4 | | `inbox-worker/handler_test.go` | Assert created sub has `RoomType: RoomTypeChannel`. | Task 4 | @@ -38,11 +38,13 @@ No store interfaces change. `room-worker`'s `SubscriptionStore` already has `Get 1. **`pkg/model` foundation** — new constants + new `Subscription.RoomType` field + model tests. Backward-compatible: existing call sites keep compiling because the new field defaults to `""`. 2. **`room-service` CreateRoom** — owner subscription carries `req.Type`. -3. **`room-worker`** — three sub literals updated: - - `processAddMembers`: hardcode `RoomTypeChannel`. - - `processRemoveIndividual`: fetch the room, stamp `RoomType` on the partial sub literal carried by the "removed" `SubscriptionUpdateEvent`. - - `processRemoveOrg`: same, with one room fetch reused across the per-account event loop. -4. **`inbox-worker`** — cross-site `handleMemberAdded` hardcodes `RoomTypeChannel` (only channel/discussion-style rooms ever produce cross-site `member_added` events, never DM/botDM). +3. **`room-worker`** — three sub literals updated, all hardcoding `RoomTypeChannel`: + - `processAddMembers`: per-account sub creation. + - `processRemoveIndividual`: partial sub literal carried by the "removed" `SubscriptionUpdateEvent`. + - `processRemoveOrg`: same, in the per-account event loop. + + Member-management ops only apply to channel rooms — room-service rejects them for any other kind, so the hardcode is safe. +4. **`inbox-worker`** — cross-site `handleMemberAdded` hardcodes `RoomTypeChannel`. The originating site's `member.add` is gated by room-service to channels only, so any cross-site `member_added` event is always for a channel room. --- @@ -101,12 +103,12 @@ This task is independently buildable: every existing call site keeps compiling b - [ ] **Step 1.3: Run the tests — expect a build failure** - ``` + ```bash make test SERVICE=pkg/model ``` Expected output (verbatim): - ``` + ```text pkg/model/model_test.go:387:3: unknown field RoomType in struct literal of type model.Subscription pkg/model/model_test.go:416:11: undefined: model.RoomTypeBotDM pkg/model/model_test.go:419:11: undefined: model.RoomTypeDiscussion @@ -147,19 +149,19 @@ This task is independently buildable: every existing call site keeps compiling b - [ ] **Step 1.6: Run the tests — expect green** - ``` + ```bash make test SERVICE=pkg/model ``` Expected: - ``` + ```text ok github.com/hmchangw/chat/pkg/model 1.0xxs ok github.com/hmchangw/chat/pkg/model/cassandra (cached) ``` - [ ] **Step 1.7: Lint clean** - ``` + ```bash make lint ``` @@ -196,12 +198,12 @@ Depends on Task 1 (`Subscription.RoomType` must exist). - [ ] **Step 2.2: Run the test — expect red** - ``` + ```bash make test SERVICE=room-service ``` Expected: - ``` + ```text --- FAIL: TestHandler_CreateRoom (0.00s) handler_test.go:51: expected owner subscription RoomType="channel", got "" ``` @@ -226,7 +228,7 @@ Depends on Task 1 (`Subscription.RoomType` must exist). - [ ] **Step 2.4: Run the test — expect green** - ``` + ```bash make test SERVICE=room-service ``` @@ -234,7 +236,7 @@ Depends on Task 1 (`Subscription.RoomType` must exist). - [ ] **Step 2.5: Lint clean** - ``` + ```bash make lint ``` @@ -259,19 +261,16 @@ Depends on Task 1 (`Subscription.RoomType` must exist). - Modify: `room-worker/handler_test.go` - `TestHandler_ProcessAddMembers` - `TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly` - - `TestHandler_ProcessRemoveMember_OwnerRemovesIndividual` - - `TestHandler_ProcessRemoveMember_OwnerRemovesOrg` - - `TestHandler_ProcessRemoveMember_CrossSiteOutbox` - - `TestHandler_ProcessRemoveIndividual_OutboxFailurePropagates` - - `TestHandler_ProcessRemoveOrg_OutboxFailurePropagates` - -Depends on Task 1. The room-worker `SubscriptionStore` interface already has `GetRoom`; no `make generate` needed. -**Design notes (from spec §4):** +Depends on Task 1. No store interface changes; no `make generate` needed. -The remove paths fetch the room once per operation and stamp `room.Type` onto the partial Subscription literal that the `SubscriptionUpdateEvent` payload carries. The fetch is **non-fatal** — on `GetRoom` error, log at warn and continue with `RoomType: ""`. The removal itself must not block on this lookup. +**Design notes:** -`processAddMembers` does NOT do a room-fetch — it hardcodes `RoomTypeChannel` because DM/botDM rooms reject `member.add` upstream. +All three sub literals hardcode `RoomTypeChannel`. Member-management ops +(`member.add`, `member.remove`, `member.role-update`) only apply to channel +rooms — room-service rejects them for any other kind before they reach the +room-worker stream. Hardcoding mirrors `processAddMembers` and avoids a +runtime `GetRoom` round-trip. - [ ] **Step 3.1: Update `TestHandler_ProcessAddMembers` — assert RoomType on every created sub** @@ -292,22 +291,11 @@ The remove paths fetch the room once per operation and stamp `room.Type` onto th }) ``` -- [ ] **Step 3.2: Update `TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly` — add `GetRoom` mock and `RoomType` assertion** - - Add a new `EXPECT` for `GetRoom` after the existing `ReconcileUserCount` expect: - - ```go - store.EXPECT(). - ReconcileUserCount(gomock.Any(), roomID).Return(nil) - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) - ``` +- [ ] **Step 3.2: Update `TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly` — assert RoomType on the published event payload** - Then after the `assert.True(t, subjSet[subject.MemberEvent(roomID)], ...)` line, add: + After the `assert.True(t, subjSet[subject.MemberEvent(roomID)], ...)` line, add: ```go - // The subscription update event should carry the RoomType from the fetched room. for _, p := range published { if p.subj != subject.SubscriptionUpdate(account) { continue @@ -318,61 +306,23 @@ The remove paths fetch the room once per operation and stamp `room.Type` onto th } ``` -- [ ] **Step 3.3: Update `TestHandler_ProcessRemoveMember_OwnerRemovesIndividual` — add `GetRoom` mock** - - After the existing `ReconcileUserCount` expect, append: - - ```go - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) - ``` - - No additional assertion needed — the previous test already asserts the RoomType payload contract. - -- [ ] **Step 3.4: Update `TestHandler_ProcessRemoveMember_OwnerRemovesOrg` — add `GetRoom` mock** - - After the existing `ReconcileUserCount(... )` expect, append: - - ```go - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) - ``` - -- [ ] **Step 3.5: Update `TestHandler_ProcessRemoveMember_CrossSiteOutbox` — add `GetRoom` mock** - - After the existing `ReconcileUserCount(... )` expect, append: - - ```go - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) - ``` - -- [ ] **Step 3.6: Update both `OutboxFailurePropagates` tests — add `GetRoom` mock** - - Both `TestHandler_ProcessRemoveIndividual_OutboxFailurePropagates` and `TestHandler_ProcessRemoveOrg_OutboxFailurePropagates` exercise the publish path, so they will also hit the new `GetRoom` call. Add this expect after `ReconcileUserCount` in each test: + This single assertion covers the contract for both `processRemoveIndividual` and `processRemoveOrg` — the shape of the partial Subscription literal is identical at both sites. - ```go - store.EXPECT(). - GetRoom(gomock.Any(), roomID). - Return(&model.Room{ID: roomID, Type: model.RoomTypeChannel}, nil) - ``` - -- [ ] **Step 3.7: Run tests — expect red** +- [ ] **Step 3.3: Run tests — expect red** - ``` + ```bash make test SERVICE=room-worker ``` - Expected failures: `TestHandler_ProcessAddMembers` (`RoomType = ""`), and the four+two remove tests fail with `missing call(s) to *main.MockSubscriptionStore.GetRoom`. + Expected failures: `TestHandler_ProcessAddMembers` (`RoomType = ""`) and `TestHandler_ProcessRemoveMember_SelfLeave_IndividualOnly` (`RoomType = ""`). -- [ ] **Step 3.8: Hardcode `RoomTypeChannel` on `processAddMembers` subs** +- [ ] **Step 3.4: Hardcode `RoomTypeChannel` on `processAddMembers` subs** In `room-worker/handler.go` find the literal inside `processAddMembers`: ```go + // RoomType is fixed to channel: room-service rejects member.add for + // any other room kind. sub := &model.Subscription{ ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, @@ -384,28 +334,18 @@ The remove paths fetch the room once per operation and stamp `room.Type` onto th } ``` -- [ ] **Step 3.9: Fetch room and stamp RoomType in `processRemoveIndividual`** +- [ ] **Step 3.5: Hardcode `RoomTypeChannel` on the partial Subscription in `processRemoveIndividual`** - In `room-worker/handler.go` find the block that begins with `now := time.Now().UTC()` followed by `// Subscription update event` (inside `processRemoveIndividual`). Insert a `GetRoom` call between them and use the fetched type when building the partial Subscription: + Inside `processRemoveIndividual`, replace the `Subscription:` field of the `SubscriptionUpdateEvent` with: ```go - now := time.Now().UTC() - - // Fetch the room so the removed-event payload carries RoomType. - // Non-fatal: on failure we proceed with an empty RoomType. - var roomType model.RoomType - if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { - slog.Warn("get room failed during remove-individual", "error", err, "roomID", req.RoomID) - } else if room != nil { - roomType = room.Type - } - - // Subscription update event + // Subscription update event. RoomType is fixed to channel: room-service + // rejects member.remove for any other room kind. subEvt := model.SubscriptionUpdateEvent{ UserID: user.ID, Subscription: model.Subscription{ RoomID: req.RoomID, - RoomType: roomType, + RoomType: model.RoomTypeChannel, User: model.SubscriptionUser{ID: user.ID, Account: req.Account}, }, Action: "removed", @@ -413,58 +353,39 @@ The remove paths fetch the room once per operation and stamp `room.Type` onto th } ``` -- [ ] **Step 3.10: Fetch room and stamp RoomType in `processRemoveOrg`** +- [ ] **Step 3.6: Hardcode `RoomTypeChannel` on the partial Subscription in `processRemoveOrg`** - In `room-worker/handler.go` find the matching `now := time.Now().UTC()` followed by `// Publish per-account subscription update and collect cross-site accounts` block (inside `processRemoveOrg`). Make the same insertion — one `GetRoom` call before the loop, reuse `roomType` per iteration: + Inside `processRemoveOrg`'s per-account loop, replace the `Subscription:` field with: ```go - now := time.Now().UTC() - - // Fetch the room once so every per-account removed event carries RoomType. - // Non-fatal: on failure we proceed with an empty RoomType. - var roomType model.RoomType - if room, err := h.store.GetRoom(ctx, req.RoomID); err != nil { - slog.Warn("get room failed during remove-org", "error", err, "roomID", req.RoomID) - } else if room != nil { - roomType = room.Type - } - - // Publish per-account subscription update and collect cross-site accounts - sectName := "" - for _, m := range toRemove { - if m.SectName != "" { - sectName = m.SectName - } - subEvt := model.SubscriptionUpdateEvent{ - Subscription: model.Subscription{ - RoomID: req.RoomID, - RoomType: roomType, - User: model.SubscriptionUser{Account: m.Account}, - }, - Action: "removed", - Timestamp: now.UnixMilli(), - } - // ...rest of the loop unchanged + subEvt := model.SubscriptionUpdateEvent{ + Subscription: model.Subscription{ + RoomID: req.RoomID, + RoomType: model.RoomTypeChannel, + User: model.SubscriptionUser{Account: m.Account}, + }, + Action: "removed", + Timestamp: now.UnixMilli(), } ``` -- [ ] **Step 3.11: Run tests — expect green** +- [ ] **Step 3.7: Run tests — expect green** - ``` + ```bash make test SERVICE=room-worker ``` Expected: `ok github.com/hmchangw/chat/room-worker 1.0xxs` -- [ ] **Step 3.12: Lint clean** +- [ ] **Step 3.8: Lint clean** - ``` + ```bash make lint ``` Expected: `0 issues.` -- [ ] **Step 3.13: Commit** +- [ ] **Step 3.9: Commit** ```bash git add room-worker/ @@ -495,12 +416,12 @@ Cross-site `member_added` events only fire for rooms that support add-member (ch - [ ] **Step 4.2: Run tests — expect red** - ``` + ```bash make test SERVICE=inbox-worker ``` Expected: - ``` + ```text --- FAIL: TestHandleEvent_MemberAdded (0.00s) handler_test.go:222: subscription RoomType = "", want "channel" ``` @@ -524,7 +445,7 @@ Cross-site `member_added` events only fire for rooms that support add-member (ch - [ ] **Step 4.4: Run tests — expect green** - ``` + ```bash make test SERVICE=inbox-worker ``` @@ -532,7 +453,7 @@ Cross-site `member_added` events only fire for rooms that support add-member (ch - [ ] **Step 4.5: Lint clean** - ``` + ```bash make lint ``` @@ -553,7 +474,7 @@ After all four tasks land, sweep the whole repo and push: - [ ] **V.1: Full test sweep** - ``` + ```bash make test ``` @@ -561,7 +482,7 @@ After all four tasks land, sweep the whole repo and push: - [ ] **V.2: Full lint sweep** - ``` + ```bash make lint ``` @@ -569,7 +490,7 @@ After all four tasks land, sweep the whole repo and push: - [ ] **V.3: Integration tests (where Docker is available)** - ``` + ```bash make test-integration SERVICE=room-worker make test-integration SERVICE=room-service make test-integration SERVICE=inbox-worker @@ -579,18 +500,18 @@ After all four tasks land, sweep the whole repo and push: - [ ] **V.4: Push to the feature branch** - ``` + ```bash git push -u origin claude/add-roomtype-subscription-Uqow3 ``` If the branch already has the previous (pre-rebase) commits on the remote, force-push with lease: - ``` + ```bash git push --force-with-lease origin claude/add-roomtype-subscription-Uqow3 ``` ## Risks and rollback - **Old subscriptions without `roomType`.** Returned as `RoomType: ""`. No code in this branch reads the field, so old documents are harmless until a future consumer relies on it. A backfill job is out of scope. -- **`GetRoom` failure during remove paths.** Logged at warn; the removal itself still succeeds and the SubscriptionUpdateEvent fires with an empty `RoomType`. Matches the pre-existing best-effort treatment of notification payloads. +- **A future room kind that supports member-management.** All three room-worker sites and inbox-worker hardcode `RoomTypeChannel` because room-service rejects member ops for non-channels today. If room-service is ever extended to allow `member.add`/`member.remove`/`member.role-update` on a different room kind (e.g., `discussion`), every hardcoded site must be revisited or the persisted `RoomType` will be wrong. Test assertions and the explanatory `// RoomType is fixed to channel: ...` comments at each site exist to surface this. - **Future creation site forgets to set `RoomType`.** Mitigated by per-site test assertions in Tasks 2-4. - **Rollback:** revert the four implementation commits in reverse order. Reverting only the model commit (Task 1) would break compilation of the call sites that use the new field — revert all four together if needed. diff --git a/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md b/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md index fca9224a5..fd9dfacef 100644 --- a/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md +++ b/docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md @@ -80,8 +80,8 @@ type Subscription struct { | Site | Source of `RoomType` | |---|---| | `room-service/handler.go` `handleCreateRoom` (owner subscription) | `req.Type` (the type the room is being created as) | -| `room-worker/handler.go` `processAddMembers` | Hardcoded `RoomTypeChannel`. DM and botDM do not support add-member. | -| `inbox-worker/handler.go` `handleMemberAdded` | Hardcoded `RoomTypeChannel`. Cross-site member_added events only fire for rooms that support add-member. | +| `room-worker/handler.go` `processAddMembers` | Hardcoded `RoomTypeChannel`. Member-management ops (add/remove/role) only apply to channel rooms — room-service rejects them for any other kind. | +| `inbox-worker/handler.go` `handleMemberAdded` | Hardcoded `RoomTypeChannel`. Cross-site `member_added` events only originate from channel rooms. | `processInvite` is intentionally absent: PR #118 ("Remove-member / role-update hardening", merged Apr 24 to main) removed the single-user @@ -91,18 +91,18 @@ in `HandleJetStreamMsg`, and the function does not exist on main. ### 4. Partial `Subscription` payloads on removed events In `room-worker/handler.go` the "removed" variants of `SubscriptionUpdateEvent` -construct a partial `Subscription` literal. Populate `RoomType` by fetching the -room once per operation (not per account): - -- `processRemoveIndividual`: call `store.GetRoom(ctx, req.RoomID)` - near the top of the function, reuse `room.Type` when building the - `SubscriptionUpdateEvent.Subscription`. -- `processRemoveOrg`: same — one `GetRoom` at the top, reuse across - the per-account event loop. - -If `GetRoom` returns an error, log at warn level and continue with -`RoomType: ""`. The removal itself must not block on this lookup; populating -the field is best-effort for the notification payload. +construct a partial `Subscription` literal. Hardcode `RoomType` to +`RoomTypeChannel` at both sites: + +- `processRemoveIndividual`: stamp `RoomType: model.RoomTypeChannel` on the + partial Subscription literal. +- `processRemoveOrg`: same — every per-account event in the loop carries + `RoomTypeChannel`. + +The hardcode mirrors `processAddMembers` and rests on the same invariant: +room-service is the single entry point for member-management requests and +rejects every kind except channel before they reach the room-worker stream. +No runtime `GetRoom` lookup is required. ### 5. `RoomTypeGroup` → `RoomTypeChannel` renames