feat: consolidate cross-site room-creation federation event#173
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates cross-site room-creation federation onto a single ChangesConsolidate Cross-Site Room-Creation Federation onto Member-Added Event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Joey0538
changed the title
Joey0538
changed the base branch from
claude/spec-consolidate-room-create-federation
to
main
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
9f473cb to
c250d68
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md`:
- Around line 251-260: The document contains a duplicated 4-step post-deploy
verification checklist (the block starting with "Create a federated DM (alice@s1
→ bob@s2)..." and listing four checks including "Query the spotlight ES
index..." and "nats stream info OUTBOX_{site}..."); remove the repeated copy so
only a single instance of that checklist remains in the rollout/post-deploy
verification section, leaving the original checklist intact and removing the
exact duplicate paragraph to avoid redundancy.
- Around line 204-220: The spec's heading "Unit only, per spec." contradicts the
integration-test changes described below; update the document to clearly state
both scopes by replacing that line with a concise summary that both unit tests
(in inbox-worker/handler_test.go — e.g.,
TestHandleMemberAdded_Channel_BuildsChannelSub,
TestHandleMemberAdded_DM_BuildsRecipientSubWithCounterpartName,
TestHandleMemberAdded_BotDM_BuildsBotSub,
TestHandleMemberAdded_DuplicateKey_IsIdempotent) are added/modified and
integration tests (in inbox-worker/integration_test.go — e.g.,
TestHandleMemberAddedDM_PersistsCorrectShape_Integration) are updated/deleted as
listed, so readers know unit and integration changes are intentional and
distinct.
In `@inbox-worker/handler_test.go`:
- Around line 1156-1173: The current test
TestHandleMemberAdded_DuplicateKey_IsIdempotent only asserts idempotent behavior
for duplicate-key (11000) bulkCreateErr; add a new test that sets
stubInboxStore.bulkCreateErr to a non-duplicate mongo.WriteException (e.g.,
WriteError with Code != 11000 or a generic error) and assert that calling
Handler.HandleEvent with a member_added OutboxEvent returns an error,
referencing the handler function handleMemberAdded via NewHandler/HandleEvent
and the stubInboxStore.bulkCreateErr field to trigger the "real store failure"
branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90745eeb-10f8-4ddb-b5dc-ee4debcf46d1
📒 Files selected for processing (9)
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/event.gopkg/model/model_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/model/model_test.go
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
c250d68 to
d998609
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
inbox-worker/integration_test.go (1)
450-451: ⚡ Quick winInclude
SiteID/DestSiteIDinOutboxEventfixtures for production-shape parity.These tests currently pass minimal events; adding routing fields makes them more robust against future handler logic that may branch on federation metadata.
Suggested test-fixture update
-evt, err := json.Marshal(model.OutboxEvent{Type: "member_added", Payload: payload}) +evt, err := json.Marshal(model.OutboxEvent{ + Type: "member_added", + SiteID: "site-A", + DestSiteID: "site-B", + Payload: payload, +})-evt, err := json.Marshal(model.OutboxEvent{Type: "member_added", Payload: payload}) +evt, err := json.Marshal(model.OutboxEvent{ + Type: "member_added", + SiteID: "site-A", + DestSiteID: "site-B", + Payload: payload, +})Also applies to: 486-487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inbox-worker/integration_test.go` around lines 450 - 451, Update the OutboxEvent test fixtures to include federation routing fields so they mirror production shape: when creating model.OutboxEvent instances (e.g., the member_added event constructed in integration_test.go where evt is built via json.Marshal(model.OutboxEvent{Type: "member_added", Payload: payload})), add SiteID and DestSiteID fields with appropriate test values (and repeat the same change for the other occurrence around lines 486–487). Ensure you set these fields on every OutboxEvent fixture used by the tests so handlers that branch on SiteID/DestSiteID see production-like events.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md`:
- Around line 166-167: Update the spec text so the testing and implementation
references match the actual changes: replace the mention of a new
search-sync-worker/spotlight_test.go and the single
TestHandleMemberAddedDM_PersistsCorrectShape_Integration entry with the two
inbox-worker integration tests added (TestHandleRoomCreatedPersistsRemoteSubs
and TestHandleRoomCreatedDM_PersistsRemoteCounterpartSub), and remove any stale
reference to the search-sync test; also update the implementation summary for
room-worker/handler.go::finishCreateRoom to state that the per-remote-site
room_created publish was removed and that RoomType and RequesterAccount were
added to the existing member_added publishes (both local-INBOX and cross-site
OUTBOX); apply the same corrections in the other duplicated sections that
currently repeat the outdated test/spec text.
---
Nitpick comments:
In `@inbox-worker/integration_test.go`:
- Around line 450-451: Update the OutboxEvent test fixtures to include
federation routing fields so they mirror production shape: when creating
model.OutboxEvent instances (e.g., the member_added event constructed in
integration_test.go where evt is built via json.Marshal(model.OutboxEvent{Type:
"member_added", Payload: payload})), add SiteID and DestSiteID fields with
appropriate test values (and repeat the same change for the other occurrence
around lines 486–487). Ensure you set these fields on every OutboxEvent fixture
used by the tests so handlers that branch on SiteID/DestSiteID see
production-like events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a0f6c20-f130-4afd-8110-f35065b8320d
📒 Files selected for processing (9)
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/event.gopkg/model/model_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/model/model_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/model/event.go
- room-worker/handler_test.go
- room-worker/integration_test.go
- room-worker/handler.go
- inbox-worker/handler_test.go
- inbox-worker/handler.go
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
d998609 to
df2359a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
room-worker/integration_test.go (1)
648-673: ⚡ Quick winAdd parity assertions for site-C
member_addedpayload fields.Line 660/Line 663 validate
RoomTypeandRequesterAccountonly for site-B. Mirroring those checks for site-C tightens the federation contract test and catches partial fan-out regressions.Suggested test addition
var memberEnvC model.OutboxEvent require.NoError(t, json.Unmarshal(memberC[0].data, &memberEnvC)) var memberPayloadC model.MemberAddEvent require.NoError(t, json.Unmarshal(memberEnvC.Payload, &memberPayloadC)) assert.ElementsMatch(t, []string{"ian"}, memberPayloadC.Accounts) +assert.Equal(t, model.RoomTypeChannel, memberPayloadC.RoomType) +assert.Equal(t, "alice", memberPayloadC.RequesterAccount) assert.Equal(t, reqID+":site-C", memberC[0].msgID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-worker/integration_test.go` around lines 648 - 673, The site-C member_added payload (memberPayloadC from memberEnvC/memberC) currently only asserts Accounts and msgID; add parity assertions mirroring site-B: check memberPayloadC.RoomType equals model.RoomTypeChannel, memberPayloadC.RoomName equals "deal team", memberPayloadC.SiteID equals "site-A", memberPayloadC.RequesterAccount equals "alice", and assert memberPayloadC.HistorySharedSince is nil so the site-C payload is validated the same as site-B.docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md (1)
180-183: ⚡ Quick winMake the idempotency section declarative (it currently contradicts itself).
Line 180 says “No change,” then Line 182 says handling must be added to
handleMemberAdded. Please rewrite this as final-state behavior only.Suggested wording cleanup
- No change. The unique index on `subscriptions.(roomId, u.account)` already handles concurrent or redelivered creates idempotently. The dedup-key fall-through fix from PR `#169` (CodeRabbit's catch) still applies — `mongo.IsDuplicateKeyError` is swallowed and execution continues so search-sync-worker's MV update still fires on replays. - - Wait — that fix is in `handleRoomCreated`. After deleting `handleRoomCreated`, the dup-key handling needs to be present in `handleMemberAdded` too. + Idempotency behavior is preserved: the unique index on `subscriptions.(roomId, u.account)` remains the source of truth, and duplicate-key errors are intentionally swallowed in `handleMemberAdded` so JetStream replays do not cause retry loops.Also applies to: 190-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md` around lines 180 - 183, Rewrite the "idempotency" section to state the final intended behavior (declarative), removing contradictory language like "No change" vs. "must be added": state that the unique index on subscriptions.(roomId, u.account) makes concurrent or redelivered creates idempotent; assert that the dedup-key fall-through fix (the mongo.IsDuplicateKeyError swallow/continue behavior introduced in PR `#169`) must be applied wherever room/member create logic runs, including both handleRoomCreated and handleMemberAdded; update the text to explicitly require that handleMemberAdded must ignore duplicate-key errors (not return them) so search-sync-worker MV updates still fire on replays, and remove any legacy phrasing that implies partial or conditional edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md`:
- Around line 180-183: Rewrite the "idempotency" section to state the final
intended behavior (declarative), removing contradictory language like "No
change" vs. "must be added": state that the unique index on
subscriptions.(roomId, u.account) makes concurrent or redelivered creates
idempotent; assert that the dedup-key fall-through fix (the
mongo.IsDuplicateKeyError swallow/continue behavior introduced in PR `#169`) must
be applied wherever room/member create logic runs, including both
handleRoomCreated and handleMemberAdded; update the text to explicitly require
that handleMemberAdded must ignore duplicate-key errors (not return them) so
search-sync-worker MV updates still fire on replays, and remove any legacy
phrasing that implies partial or conditional edits.
In `@room-worker/integration_test.go`:
- Around line 648-673: The site-C member_added payload (memberPayloadC from
memberEnvC/memberC) currently only asserts Accounts and msgID; add parity
assertions mirroring site-B: check memberPayloadC.RoomType equals
model.RoomTypeChannel, memberPayloadC.RoomName equals "deal team",
memberPayloadC.SiteID equals "site-A", memberPayloadC.RequesterAccount equals
"alice", and assert memberPayloadC.HistorySharedSince is nil so the site-C
payload is validated the same as site-B.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6b82080-fc2d-4900-a833-9813eae0430d
📒 Files selected for processing (9)
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/event.gopkg/model/model_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/model/model_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/model/event.go
- room-worker/handler_test.go
- inbox-worker/integration_test.go
- room-worker/handler.go
- inbox-worker/handler_test.go
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
df2359a to
fbc6c7d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md (1)
38-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve the search-sync test contradiction in the spec.
Line 39 says a new regression test was added, but later sections state no new search-sync-worker tests were added. Keep one consistent statement to avoid confusion in review and rollout notes.
📝 Suggested doc fix
-- **Search-sync-worker code changes.** Zero structural change required. One regression test added to lock in the latent-bug fix. +- **Search-sync-worker code changes.** Zero structural change required. No new tests were added; existing spotlight coverage already asserts `roomType`.Also applies to: 176-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md` around lines 38 - 39, The doc contains a contradiction about search-sync-worker tests: the heading "Search-sync-worker code changes. Zero structural change required. One regression test added to lock in the latent-bug fix." conflicts with later lines saying no new search-sync-worker tests were added; pick one consistent statement and update all occurrences (e.g., the heading and the later section at lines referencing "search-sync-worker tests" or "regression test") so the spec either consistently declares that a regression test was added or consistently declares no new tests were added, and adjust any accompanying explanation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md`:
- Around line 38-39: The doc contains a contradiction about search-sync-worker
tests: the heading "Search-sync-worker code changes. Zero structural change
required. One regression test added to lock in the latent-bug fix." conflicts
with later lines saying no new search-sync-worker tests were added; pick one
consistent statement and update all occurrences (e.g., the heading and the later
section at lines referencing "search-sync-worker tests" or "regression test") so
the spec either consistently declares that a regression test was added or
consistently declares no new tests were added, and adjust any accompanying
explanation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1e8f3e3-156b-4824-93a8-5fe889250ba7
📒 Files selected for processing (9)
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/event.gopkg/model/model_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/model/model_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/model/event.go
- inbox-worker/handler.go
- inbox-worker/handler_test.go
- room-worker/handler.go
- room-worker/handler_test.go
- room-worker/integration_test.go
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
fbc6c7d to
22c0346
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md`:
- Around line 146-148: The example's handleMemberAdded block currently returns
any error from h.store.BulkCreateSubscriptions but the idempotency section
expects duplicate-key errors to be ignored for JetStream replay; update
handleMemberAdded to detect and swallow duplicate-key/unique-constraint errors
from BulkCreateSubscriptions (while returning other errors) so the main example
and idempotency snippet are consistent, then remove or condense the separate
idempotency snippet (lines ~182–188) to avoid duplication; refer to the
handleMemberAdded function and the h.store.BulkCreateSubscriptions call when
making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6805e834-143d-4e7d-b6b1-d8c3e5b5ced3
📒 Files selected for processing (9)
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/event.gopkg/model/model_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/model/model_test.go
✅ Files skipped from review due to trivial changes (1)
- room-worker/handler_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/model/event.go
- room-worker/handler.go
- inbox-worker/handler_test.go
- inbox-worker/handler.go
- inbox-worker/integration_test.go
- room-worker/integration_test.go
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
22c0346 to
9bd35c1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
inbox-worker/integration_test.go (1)
439-448: ⚡ Quick winPopulate
JoinedAtin themember_addedfixtures to avoid epoch defaults.Both new event fixtures omit
JoinedAt, so this path currently exercisesJoinedAt=0(1970-01-01) instead of realistic create-time behavior.💡 Proposed test-fixture update
payload, err := json.Marshal(model.MemberAddEvent{ Type: "member_added", RoomID: "r_xyz", RoomName: "deal team", RoomType: model.RoomTypeChannel, Accounts: []string{"bob", "ian"}, SiteID: "site-A", RequesterAccount: "alice", + JoinedAt: time.Now().UTC().UnixMilli(), Timestamp: time.Now().UTC().UnixMilli(), })payload, err := json.Marshal(model.MemberAddEvent{ Type: "member_added", RoomID: roomID, RoomName: "", RoomType: model.RoomTypeDM, Accounts: []string{"bob"}, SiteID: "site-A", RequesterAccount: "alice", + JoinedAt: time.Now().UTC().UnixMilli(), Timestamp: time.Now().UTC().UnixMilli(), })Also applies to: 480-489
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inbox-worker/integration_test.go` around lines 439 - 448, The member_added test fixtures construct model.MemberAddEvent without setting JoinedAt, causing JoinedAt to be 0; update the fixtures (the MemberAddEvent instances used in the member_added tests) to set JoinedAt to a realistic timestamp (e.g., time.Now().UTC().UnixMilli() or the same Timestamp value used) so the tests no longer exercise epoch defaults; ensure you update all occurrences (including the second fixture referenced near lines 480-489) to assign JoinedAt on the struct literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@inbox-worker/integration_test.go`:
- Around line 439-448: The member_added test fixtures construct
model.MemberAddEvent without setting JoinedAt, causing JoinedAt to be 0; update
the fixtures (the MemberAddEvent instances used in the member_added tests) to
set JoinedAt to a realistic timestamp (e.g., time.Now().UTC().UnixMilli() or the
same Timestamp value used) so the tests no longer exercise epoch defaults;
ensure you update all occurrences (including the second fixture referenced near
lines 480-489) to assign JoinedAt on the struct literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7730e5b3-f2cb-481e-8ac9-87b9603ef2cc
📒 Files selected for processing (9)
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/event.gopkg/model/model_test.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/model/model_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- room-worker/handler_test.go
- inbox-worker/handler.go
- pkg/model/event.go
- room-worker/integration_test.go
- room-worker/handler.go
- inbox-worker/handler_test.go
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
9bd35c1 to
79283a0
Compare
Drop the redundant outbox.{origin}.to.{remote}.room_created event in
favor of the existing outbox.{origin}.to.{remote}.member_added event
(added in PR #169) doing double duty: drive sub creation in inbox-worker
AND MV update in search-sync-worker, mirroring the add-members path
which already works that way since PR #145.
Extend MemberAddEvent with RoomType + RequesterAccount so
inbox-worker.handleMemberAdded can build correctly-shaped DM/botDM subs
via the existing helpers (subscriptionName / rolesForType /
subscriptionIsSubscribed) instead of needing a separate handleRoomCreated
path.
Full removal of room_created event, model, handler, and tests. Incidental
benefit: heals a latent search-sync-worker bug where the spotlight ES
doc's roomType field has been empty since PR #145 because today's
MemberAddEvent wire format doesn't carry RoomType.
https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp
- Line 160: "consts" -> "constants" (style) - Line 208: "sub shape" -> "sub-shape" (hyphenation) - Rollout section: rewrite to honestly document the no-fully-safe-single-PR-deploy-order issue CodeRabbit flagged. Walks both deploy orders showing the malformed-DM window each produces. Presents three options (A: ship as-is, B: 2-PR split, C: single PR + follow-up cleanup) with a recommendation for option C. Marks this as an open question requiring user input before implementation begins. https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp
User confirmed cross-site federation is not yet integrated end-to-end, so the theoretical mixed-version DM-sub-malformation window has no real-world incidence today. Reverting the spec to option (A): single PR ships both publisher and consumer changes; deploy order (room-worker first) is defensive rather than strictly required. Trims the option-discussion text and rolls the deploy-window risk into the Risks section with explicit acknowledgment that it's theoretical under current operational reality. https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp
Single cross-site event for room creation: outbox.{origin}.to.{remote}.
member_added does double duty — drives sub creation in inbox-worker
(with correct DM/botDM/channel shapes) AND MV update in
search-sync-worker. Drops the redundant room_created event entirely.
Schema (pkg/model/event.go):
- MemberAddEvent gains RoomType + RequesterAccount (both omitempty).
- Delete RoomCreatedOutbox struct.
- Delete OutboxTypeRoomCreated constant.
- MessageTypeRoomCreated stays — distinct system-message-type constant
used by room-worker's publishChannelSysMessages, unrelated to
federation.
Consumer (inbox-worker/handler.go):
- handleMemberAdded dispatches on event.RoomType. Empty RoomType
defaults to RoomTypeChannel for backward-compat with pre-deploy
publishers that didn't set the field.
- subscriptionName / subscriptionIsSubscribed helpers refactored to
take primitives (roomType, roomName, requesterAccount, *user)
instead of *RoomCreatedOutbox, so handleMemberAdded can call them.
- Duplicate-key BulkCreateSubscriptions errors swallowed (replay
after a crashed prior delivery is idempotent — matches PR #169 fix).
- handleRoomCreated function deleted.
- case model.MessageTypeRoomCreated arm in HandleEvent switch deleted.
Publisher (room-worker/handler.go):
- finishCreateRoom: delete the per-remote-site room_created OUTBOX
publish. Cross-site member_added publish now carries RoomType +
RequesterAccount.
- finishCreateRoom local INBOX publish: same fields populated for
consistency (search-sync-worker reads them).
- processAddMembers: populate RoomType + RequesterAccount on all
three member_added publishes (UI fan-out, local INBOX, cross-site
OUTBOX). Channels-only path, but consistent shape avoids surprises.
- publishSyncDMOutbox: switch from room_created to member_added with
the full new schema.
Tests:
- inbox-worker/handler_test.go: replace 5 TestHandleRoomCreated* tests
with TestHandleMemberAdded_DM/BotDM/Channel/EmptyRoomType/
DuplicateKey cases. Helpers refactored to match new signatures.
- inbox-worker/integration_test.go: replace 2 room_created integration
tests with member_added equivalents going through HandleEvent.
- room-worker/handler_test.go + integration_test.go: assertions on
cross-site outboxes now look for OutboxMemberAdded subjects with
full RoomType + RequesterAccount payload.
Incidental fix: search-sync-worker.spotlight.go has been writing an
empty `roomType` field to the spotlight ES doc since PR #145 because
MemberAddEvent's wire format didn't carry RoomType. Once room-worker
starts populating RoomType, the spotlight doc gets correct roomType
for the first time. No code change in search-sync-worker; existing
TestSpotlightCollection_BuildAction_MemberAdded asserts the correct
value.
Spec: docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md
https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp
Joey0538
force-pushed
the
claude/impl-consolidate-room-create-federation
branch
from
79283a0 to
88f7c66
Compare
general-lex
pushed a commit
that referenced
this pull request
May 20, 2026
…tract PR #173 (federation consolidation) reshaped model.CreateRoomRequest and moved the room-creation subject from chat.user.<account>.request.rooms.create to chat.user.<account>.request.room.<siteID>.create. Update the "creates channel room" step accordingly: - Use subject.RoomCreate(account, siteID) — the wildcard room-service now subscribes to. - Drop removed fields Type/CreatedBy/CreatedByAccount/SiteID/Members from the payload. Send only Name + RequesterAccount, which the new handler classifies as a channel (requesterAccount is parsed from the subject; user/org/channel lists empty = channel by Name). Restores compilation of the godog test binary; all 7 scenarios are discovered and execute (failing at network as expected against the example.invalid stub URLs). Note: the new handler also requires the requester to exist in Mongo via store.GetUser — a fixture the suite cannot establish in Part-1. The scenario will surface ErrUserNotFound (HandlerError class) when run against a live stack; a follow-up should either tag it @blindSpot:room-create-needs-user-fixture or add the Mongo fixture once Part-2 state-observation primitives land. https://claude.ai/code/session_0139upFqMPspygX8XqTjpRN1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates the cross-site room-creation federation event:
outbox.{origin}.to.{remote}.member_addeddoes double duty — drives sub creation ininbox-worker(with correct DM/botDM/channel shapes) AND MV update insearch-sync-worker. The redundantroom_createdevent is removed entirely.Originated from @mliu33's review on PR #169: the per-room-creation 2-event federation was a smell.
This PR contains both the spec doc and the implementation in one branch.
The bug, in one sentence
Every room creation emitted two cross-site events per remote site:
room_created(consumed byinbox-workerfor sub creation) andmember_added(consumed bysearch-sync-workerfor MV update). Both carried mostly-overlapping payloads. With one small schema extension onMemberAddEvent, both consumers can share a single event.Schema change —
pkg/model/event.goAdd two
omitemptyfields toMemberAddEvent:Delete
RoomCreatedOutboxstruct andOutboxTypeRoomCreatedconstant.MessageTypeRoomCreatedstays (distinct constant used for the chat-history sys-message, same string value but unrelated).Consumer change —
inbox-worker/handler.gohandleMemberAddednow dispatches onevent.RoomType:Empty
RoomTypedefaults to channel for backward-compat.subscriptionName/subscriptionIsSubscribedhelpers refactored to take primitives instead of*RoomCreatedOutbox.handleRoomCreatedfunction and its switch arm inHandleEventdeleted.mongo.IsDuplicateKeyErrorswallowed so JetStream replays after a crashed prior delivery are idempotent (carries over the dup-key fix from #169).Publisher change —
room-worker/handler.gofinishCreateRoom: delete the per-remote-siteroom_createdOUTBOX publish; the existingmember_addedcross-site publish now populatesRoomType: room.TypeandRequesterAccount: requester.Account.processAddMembers: populate the two new fields on all threemember_addedemissions (UI fan-out, local INBOX, cross-site OUTBOX).publishSyncDMOutbox: switch fromroom_createdtomember_addedwith the full new schema.Incidental fix
search-sync-worker/spotlight.go:120writesRoomType: string(evt.RoomType)into the spotlight ES doc, butMemberAddEvent's wire format didn't carryRoomTypeuntil this PR. Once room-worker populates it, the spotlight typeahead UI gets correct room-type info on freshly-indexed rooms for the first time. No code change in search-sync-worker; existingTestSpotlightCollection_BuildAction_MemberAddedalready assertsassert.Equal(t, "channel", doc["roomType"])viabaseInboxMemberEvent().Rollout
Cross-site federation isn't yet integrated end-to-end, so the theoretical mixed-version DM-sub-malformation window during a rolling deploy has no real-world incidence today. Defensive deploy order (room-worker first) recommended but not strictly required. See spec § Rollout for full detail.
Tests
Unit tests only, mirroring #145 / #169 precedent:
inbox-worker/handler_test.go: 5TestHandleRoomCreated*cases replaced withTestHandleMemberAdded_{DM,BotDM,Channel,EmptyRoomType,DuplicateKey}. Helpers refactored to match new primitive signatures.inbox-worker/integration_test.go: 2room_createdintegration tests rewritten to go throughHandleEventwithmember_addedpayloads.room-worker/handler_test.go+integration_test.go: outbox-payload assertions look forOutboxMemberAddedsubjects with fullRoomType+RequesterAccountpayload. Field-by-fieldroom_createdchecks dropped.Diff size
pkg/model/event.goinbox-worker/handler.goroom-worker/handler.go8 code files, +190 / -367 = net -177 LOC. Plus the spec doc (+267).
Test plan
make lintcleanmake testclean (full repo)go vet -tags integration ./...cleansubscriptionsdoc hasName="alice",Roles=nil,IsSubscribed=false,RoomType=dm. Confirm spotlightroomTypefield is populated.Files
docs/superpowers/specs/2026-05-12-consolidate-room-create-federation-design.md(NEW)pkg/model/event.goinbox-worker/handler.go,handler_test.go,integration_test.goroom-worker/handler.go,handler_test.go,integration_test.goWhat's NOT in this PR
RoomCreatedOutbox(no out-of-tree consumer exists pergrep).MessageTypeRoomCreatedconstant removal (kept; it's the chat-history sys-message type, unrelated).Note on commit history
This PR was originally split as PR #172 (spec only) + PR #173 (implementation stacked on the spec branch). Combined here per request for review convenience. PR #172 closed in favor of this one.
https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation