feat(create-room): DM, botDM, and channel creation across sites#142
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:
📝 WalkthroughWalkthroughAdds a complete create-room design and implementation: new NATS subjects and parsers, canonical create publishing, room-service validation/ID logic, room-worker canonical processing and idempotent persistence, inbox-worker cross-site subscription materialization, store/index additions, model/event shape updates, and extensive tests. ChangesCreate-Room end-to-end
Sequence Diagram(s)sequenceDiagram
participant Client
participant RoomService as Room-Service
participant RoomWorker as Room-Worker
participant DB as MongoDB
participant Inbox as Inbox-Worker
Client->>RoomService: NATS request chat.user.{acct}.request.room.{site}.create + X-Request-ID
activate RoomService
RoomService->>RoomService: Parse subject, validate X-Request-ID, classify (DM/botDM/channel)
RoomService->>DB: GetUser / GetApp / FindDMSubscription (dedupe)
RoomService->>RoomService: Compute roomID (deterministic for DM) or generate channel ID
RoomService->>RoomWorker: Publish canonical create (chat.room.canonical.{site}.create)
RoomService->>Client: Reply accepted (CreateRoomReply)
deactivate RoomService
activate RoomWorker
RoomWorker->>DB: CreateRoom (idempotent insert), Create subscriptions, optional room_members
RoomWorker->>DB: ReconcileMemberCounts (compute UserCount & AppCount)
RoomWorker->>RoomWorker: Publish subscription.update and channel sys-messages
RoomWorker->>Inbox: Emit per-remote-site room_created outbox events
deactivate RoomWorker
activate Inbox
Inbox->>Inbox: Unmarshal outbox, require X-Request-ID
Inbox->>DB: Resolve local users
Inbox->>DB: Bulk-create remote subscriptions (ignore duplicate-key)
deactivate Inbox
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
room-worker/handler.go (1)
711-718:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate explicit
History.SharedSinceintoMemberAddEventtoo.The subscription write path uses
req.History.SharedSincewhen present, but the local/outboxMemberAddEventalways publishesreq.Timestamp. Remote inbox workers will therefore materialize a differentHistorySharedSincethan the home-site subscriptions whenever the request supplied an explicit cutoff.Suggested fix
- historySharedSince := historySharedSincePtr(req.History.Mode, req.Timestamp, req.RoomID) + var historySharedSince *int64 + if req.History.Mode == model.HistoryModeNone { + ts := req.Timestamp + if req.History.SharedSince != nil { + ts = *req.History.SharedSince + } + historySharedSince = &ts + }Also applies to: 765-773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/handler.go` around lines 711 - 718, The MemberAddEvent currently sets HistorySharedSince to req.Timestamp instead of the explicit cutoff; change the event to use the computed historySharedSince (from historySharedSincePtr(req.History.Mode, req.Timestamp, req.RoomID) or directly req.History.SharedSince when present) for MemberAddEvent.HistorySharedSince so remote inbox workers materialize the same History.SharedSince as the subscription path; apply the same fix in the other occurrence around the MemberAddEvent creation at the 765-773 block so both local/outbox and remote paths propagate the explicit History.SharedSince consistently.
🧹 Nitpick comments (3)
room-service/integration_test.go (1)
1093-1097: ⚡ Quick winAvoid
time.Sleepin the timeout responder.A subscriber that intentionally never replies exercises the same client-timeout path without adding a fixed 400ms delay or relying on sleep-based coordination.
Suggested fix
// Sleep just past the 200ms client timeout so the responder doesn't outlive the test // and flag as a goroutine leak under -race/leak detectors. sub, err := nc.Subscribe(subject.MemberList("alice", "source", "site-b"), func(m *nats.Msg) { - time.Sleep(400 * time.Millisecond) - _ = m.Respond([]byte(`{}`)) + _ = m // Intentionally do not respond; requester should hit its timeout. })As per coding guidelines "Never use 'time.Sleep' for goroutine synchronization — use proper sync primitives".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-service/integration_test.go` around lines 1093 - 1097, Replace the sleep-and-respond handler with a non-replying handler and explicitly unsubscribe when the test is done: change the nc.Subscribe callback for subject.MemberList("alice", "source", "site-b") to a handler that does not call m.Respond (i.e., returns immediately) to exercise the client-timeout path, and after the test-triggering request/expectation completes call sub.Unsubscribe() (or use sub.AutoUnsubscribe/Drain) to ensure the subscriber does not outlive the test instead of using time.Sleep.pkg/subject/subject_test.go (1)
104-118: ⚡ Quick winAdd wildcard-token negatives for the new room-create subject helpers.
The new
RoomCreate/ParseRoomCreateSubjectcoverage never exercises accounts containing*or>, so the subject-token guard can regress on this new surface without a test failing.Based on learnings "add a single shared guard/validator for any function that accepts an
accounttoken ... reject account values containing NATS wildcard characters*and>".Also applies to: 263-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/subject/subject_test.go` around lines 104 - 118, Add negative tests that assert the account-token guard rejects NATS wildcard characters for the new room-create helpers: add t.Run cases (e.g., "RoomCreateRejectsWildcardAccount" and "ParseRoomCreateSubjectRejectsWildcardAccount") that call subject.RoomCreate and subject.ParseRoomCreateSubject with account values containing "*" and ">" and assert the functions reject them (use testing/quick assert.Panics or defer+recover to expect a panic, or the function's error-return contract if it returns an error) so the guard around account tokens is exercised for wildcard inputs.room-service/handler_test.go (1)
1895-2221: ⚡ Quick winAdd a malformed JSON case for
handleCreateRoom.This suite covers semantic validation, but it never exercises the
json.Unmarshalfailure path inhandleCreateRoom. A tiny{not jsoncase would pin the"invalid request"behavior and close the required malformed-input coverage for this handler.Based on learnings: Applies to
**/*handler_test.go:Every handler method must have tests for: valid input, invalid/malformed input, store errors, and edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-service/handler_test.go` around lines 1895 - 2221, Add a test that sends malformed JSON to handleCreateRoom to exercise the json.Unmarshal failure path: create a gomock controller and NewMockRoomStore, instantiate Handler (siteID "site-a"), call h.handleCreateRoom(ctxWithReqID(), createRoomSubj("alice","site-a"), []byte("{not json")), assert it returns an error and that the error matches errInvalidRequest (or contains "invalid request") so the invalid-input branch is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-28-create-room-design.md`:
- Around line 729-734: The spec is inconsistent about channel-name handling:
update resolveRoomName to treat channel like dm/botDM and return req.Name
verbatim (remove truncateRunes and any truncation language), and revise Scenario
C and the later block (around 1282–1288) to remove any mention of worker
auto-generating or truncating channel names; instead assert that the client must
supply a non-empty req.Name and room-service enforces errChannelNameRequired, so
the worker can rely on req.Name unchanged.
- Around line 1075-1076: Update the inbox idempotency paragraphs to reflect the
UUIDv7 subscription-ID decision: remove references to deterministic IDs produced
by MessageIDFromRequestID and instead state new subscriptions use
idgen.GenerateUUIDv7(); explain that redelivery safety relies on the unique
index on (roomId, u.account) and that duplicate-key errors on bulk insert should
be treated as success-on-redelivery. Apply this change to the affected sections
(around the current lines and the other noted spots) so all wording consistently
references subscription-ID generation via idgen.GenerateUUIDv7(), the (roomId,
u.account) unique index, and the duplicate-key = success-on-redelivery behavior.
In `@inbox-worker/handler_test.go`:
- Around line 800-827: The bot-detection logic only checks for the ".bot" suffix
so webhook-style bots like "p_*" are missed; update subscriptionSidebarName and
subscriptionIsSubscribed to treat accounts starting with "p_" as bots (e.g., use
strings.HasSuffix(account, ".bot") || strings.HasPrefix(account, "p_") or a
shared helper like IsBot) and adjust the tests in TestSubscriptionSidebarName
and TestSubscriptionIsSubscribed to add assertions covering a "p_" bot account
to ensure both branches use the bot-side fields correctly.
In `@pkg/subject/subject.go`:
- Around line 357-367: ParseRoomCreateSubject currently accepts account tokens
that include NATS wildcards; add a validation guard to reject tokens containing
'*' or '>' and return false instead of accepting them. Implement a shared helper
(e.g., IsValidAccountToken or ValidateAccountToken) that checks the account
token does not contain '*' or '>' and use it in ParseRoomCreateSubject (validate
parts[2] before returning it). Ensure the helper is reused by any other
subject-parsing functions that accept an account token to centralize the rule.
In `@room-service/handler.go`:
- Around line 284-289: The capacity check uses newCount from
h.store.CountNewMembers (based on allUsers/allOrgs) but omits the channel
creator who is implicitly added; update the check to account for the creator by
either including the creator in the inputs to h.store.CountNewMembers or by
adding 1 to newCount (e.g., effectiveCount := newCount + 1 when the requester is
not in the counted lists) before comparing to h.maxRoomSize so the room never
exceeds capacity once the creator subscription materializes.
- Around line 241-265: The handler currently checks bot availability via
h.store.GetApp and app.Assistant.Enabled before deduplicating with
h.store.FindDMSubscription, causing existing bot DMs to be rejected if the bot
is disabled; change the flow so you first call
h.store.FindDMSubscription(requester.Account, other.Account) and if it returns
an existing subscription return newDMExistsError(existing.RoomID) (handling
non-found vs other errors as currently done), and only after confirming no
existing DM proceed to call h.store.GetApp and set req.AppName and enforce
errBotNotAvailable; preserve the existing error wrapping behavior and keep
req.ResolvedUsers/RoomID logic unchanged.
- Around line 150-156: The handler currently rejects requests when
natsutil.RequestIDFromContext(ctx) returns empty; instead, when requestID == ""
call idgen.GenerateRequestID() to mint a server-side request ID and use that
value; then validate the resulting requestID with idgen.IsValidUUID and only
return errInvalidRequestID if validation fails (remove the errMissingRequestID
branch). Update any context-wrapping logic (e.g., wrappedCtx or the code that
sets requestID in the request handling path) to ensure the generated request ID
is propagated for downstream logging/tracing.
In `@room-service/helper.go`:
- Around line 85-94: determineRoomType currently only checks
strings.HasSuffix(req.Users[0], ".bot") so accounts matching the repo's other
bot pattern (p_...) are misclassified; replace that suffix check with the shared
bot predicate (e.g., IsBotUser or whatever project-wide helper exists) when
inspecting req.Users[0] so both ".bot" and "p_..." style bot accounts return
model.RoomTypeBotDM (leave the rest of the function logic intact and keep
returning model.RoomTypeDM for non-bots).
In `@room-service/store_mongo.go`:
- Around line 82-97: The dm/botDM dedup index (dmDedupIndex) is not unique, so
simultaneous inserts can create duplicate (u.account, name, roomType) documents;
update the IndexModel options used in s.subscriptions.Indexes().CreateOne to
mark the index as unique by calling SetUnique(true) on the options.Index() chain
for dmDedupIndex (still keep the partial filter for model.RoomTypeDM and
model.RoomTypeBotDM and the existing SetName).
In `@room-worker/handler.go`:
- Around line 995-1003: The code currently only validates returned users from
h.store.FindUsersByAccounts but doesn't check for missing accounts; update the
logic after calling h.store.FindUsersByAccounts to verify that every account in
the input accounts slice has a corresponding user in the returned users slice
(e.g., by building a map from users[i].Account to user and comparing counts or
iterating accounts to ensure presence). If any account is missing, return a
permanent error that names the missing account(s) (using newPermanent) instead
of proceeding; keep the existing per-user name-field checks
(EngName/ChineseName) for the found users. Ensure you reference
h.store.FindUsersByAccounts, the users slice, the accounts variable, and
newPermanent when making the change.
- Around line 907-918: On duplicate-key from h.store.CreateRoom, strengthen the
replay-equivalence check after fetching existing via h.store.GetRoom: in
addition to comparing existing.Type and existing.SiteID, also compare
identifying fields such as existing.Name and existing.CreatedBy (and any other
immutable identity fields used for channel uniqueness) and only treat it as
redelivery (assign room = existing and continue) if all these fields match;
otherwise return a permanent room ID collision error (similar to the existing
newPermanent) to avoid appending subscriptions/messages to the wrong room.
In `@room-worker/store_mongo.go`:
- Around line 57-103: In ReconcileMemberCounts, after iterating the cursor
(cur.Next) check cur.Err() and handle any iteration/decoding error before using
the potentially-zeroed counts; specifically, after the cur.Next/Decode block
verify cur.Err() and return a wrapped error (or the decode error) if non-nil so
that s.rooms.UpdateOne is not called with stale zero values for
counts.UserCount/counts.AppCount when the cursor iteration failed.
- Around line 140-153: In ListNewMembersForNewRoom, distinguish an empty
aggregation result from a cursor failure by checking the cursor error after
attempting to read: after calling cur.Next(ctx), if it returns false call
cur.Err() and if non-nil return a wrapped error (e.g. "list new members for new
room: %w") instead of silently returning nil, nil; only return nil, nil when
cur.Err() is nil (true empty result). Keep the existing cur.Close(ctx) defer and
the cur.Decode(&doc) path unchanged.
---
Outside diff comments:
In `@room-worker/handler.go`:
- Around line 711-718: The MemberAddEvent currently sets HistorySharedSince to
req.Timestamp instead of the explicit cutoff; change the event to use the
computed historySharedSince (from historySharedSincePtr(req.History.Mode,
req.Timestamp, req.RoomID) or directly req.History.SharedSince when present) for
MemberAddEvent.HistorySharedSince so remote inbox workers materialize the same
History.SharedSince as the subscription path; apply the same fix in the other
occurrence around the MemberAddEvent creation at the 765-773 block so both
local/outbox and remote paths propagate the explicit History.SharedSince
consistently.
---
Nitpick comments:
In `@pkg/subject/subject_test.go`:
- Around line 104-118: Add negative tests that assert the account-token guard
rejects NATS wildcard characters for the new room-create helpers: add t.Run
cases (e.g., "RoomCreateRejectsWildcardAccount" and
"ParseRoomCreateSubjectRejectsWildcardAccount") that call subject.RoomCreate and
subject.ParseRoomCreateSubject with account values containing "*" and ">" and
assert the functions reject them (use testing/quick assert.Panics or
defer+recover to expect a panic, or the function's error-return contract if it
returns an error) so the guard around account tokens is exercised for wildcard
inputs.
In `@room-service/handler_test.go`:
- Around line 1895-2221: Add a test that sends malformed JSON to
handleCreateRoom to exercise the json.Unmarshal failure path: create a gomock
controller and NewMockRoomStore, instantiate Handler (siteID "site-a"), call
h.handleCreateRoom(ctxWithReqID(), createRoomSubj("alice","site-a"),
[]byte("{not json")), assert it returns an error and that the error matches
errInvalidRequest (or contains "invalid request") so the invalid-input branch is
covered.
In `@room-service/integration_test.go`:
- Around line 1093-1097: Replace the sleep-and-respond handler with a
non-replying handler and explicitly unsubscribe when the test is done: change
the nc.Subscribe callback for subject.MemberList("alice", "source", "site-b") to
a handler that does not call m.Respond (i.e., returns immediately) to exercise
the client-timeout path, and after the test-triggering request/expectation
completes call sub.Unsubscribe() (or use sub.AutoUnsubscribe/Drain) to ensure
the subscriber does not outlive the test instead of using time.Sleep.
🪄 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: 99686275-fbf3-45dd-ab27-5f20f9053249
📒 Files selected for processing (31)
docs/superpowers/plans/2026-04-28-create-room.mddocs/superpowers/specs/2026-04-28-create-room-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/app.gopkg/model/error.gopkg/model/event.gopkg/model/member.gopkg/model/model_test.gopkg/model/room.gopkg/model/subscription.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
Collaborator
Author
|
Applied 11 of the 13 CodeRabbit findings in Correctness fixes
Tests added
Spec corrections
Skipped — with reasoning
Lint clean. Unit tests pass. Integration tests pass for room-service (76.8s), room-worker (9.9s), inbox-worker (4.1s). Generated by Claude Code |
vjauhari-work
force-pushed
the
claude/implement-create-room-ePJUK
branch
from
863b83e to
00b612a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
room-worker/handler_test.go (1)
887-920:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate this test to the new permanent-error contract.
This case still expects
processAddMembersto skip"ghost"and succeed, but the current behavior treats partialFindUsersByAccountsresults as a permanent"user not found"failure. As written, the test now codifies the old contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/handler_test.go` around lines 887 - 920, Update TestHandler_ProcessAddMembers_UserNotFound to follow the new permanent-error contract: when store.FindUsersByAccounts returns only a subset (missing "ghost"), do not expect BulkCreateSubscriptions, ReconcileMemberCounts or HasOrgRoomMembers to be called; instead call h.processAddMembers and assert it returns an error that is classified as a permanent "user not found" failure (use the project's permanent-error predicate or the model helper you have for checking permanent errors). Keep the mock for FindUsersByAccounts returning only "bob" and adjust the expectations to verify no further subscription creation occurs and that the returned error from processAddMembers is a permanent error.
♻️ Duplicate comments (1)
docs/superpowers/specs/2026-04-28-create-room-design.md (1)
8-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the channel-name contract consistent throughout the spec.
The summary still says channel creation is possible “optionally with a name,” and Scenario C still documents an auto-generated room name. The implementation and the rest of this spec now require a client-supplied channel name and preserve it verbatim, so these sections are still advertising the old contract.
Also applies to: 1277-1302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-28-create-room-design.md` around lines 8 - 13, Update the spec to make the channel-name contract explicit and consistent: change the summary line describing "channel" so it states that a client-supplied channel name is required and preserved verbatim (no auto-generation), and update Scenario C to remove references to an auto-generated name and instead show the client providing and the system preserving the given name; ensure any other references in the document that say "optionally with a name" or imply auto-generation are modified to reflect the required client-supplied name behavior so the create-room request/response contract is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inbox-worker/handler.go`:
- Around line 250-272: The handler for the room_created materialization
currently calls h.store.FindUsersByAccounts and proceeds even if some requested
accounts are missing, which causes silent partial state; update the logic after
calling FindUsersByAccounts to detect when len(users) != len(data.Accounts),
compute which account IDs from data.Accounts were not returned, and return a
non-nil error (including the missing account IDs and context like
RoomID/HomeSiteID) so the caller can retry/fail the materialization instead of
silently creating partial subscriptions; change the loop that builds subs
(subscription creation using idgen.GenerateUUIDv7(), model.Subscription,
subscriptionName, subscriptionSidebarName, subscriptionIsSubscribed,
rolesForType) to only run after the presence check succeeds.
In `@pkg/model/model_test.go`:
- Around line 1756-1758: The test currently discards the json.Marshal error for
r2 (AsyncJobResult), which can hide serialization failures; change the marshal
call to capture the error (raw2, err := json.Marshal(r2)) and assert
require.NoError(t, err) before using raw2, keeping the existing
assert.NotContains(t, string(raw2), `"roomId"`) and referencing the
AsyncJobResult construction and json.Marshal call so the change is localized to
that test block.
In `@room-service/handler.go`:
- Around line 287-297: The capacity check overcounts the requester because
CountNewMembers is called with an empty exclude and may already include
requesterAccount; update the call to h.store.CountNewMembers to pass
requesterAccount (the requester's account ID) as the exclude parameter so the
returned newCount does not include them, then compute totalMembers := 1 +
newCount and compare against h.maxRoomSize as before; change the call site where
newCount is assigned (CountNewMembers(..., "", ...)) to use the requesterAccount
variable so the owner is only added once.
In `@room-worker/handler.go`:
- Around line 595-600: The code currently overwrites a caller-provided
req.History.SharedSince by recomputing HistorySharedSince from req.Timestamp
(and acceptedAt) later, causing inconsistent history cutoffs; update the logic
in the handler so that when req.History.SharedSince is non-nil you set
sub.HistorySharedSince = &histTime using
time.UnixMilli(*req.History.SharedSince).UTC() and do not replace it with a
recomputed time from req.Timestamp or acceptedAt; apply the same change in the
other affected blocks (the sections around the recompute at req.Timestamp /
acceptedAt referenced by lines 711-719 and 765-773) so HistorySharedSince
remains the explicit caller-provided value whenever req.History.SharedSince !=
nil.
---
Outside diff comments:
In `@room-worker/handler_test.go`:
- Around line 887-920: Update TestHandler_ProcessAddMembers_UserNotFound to
follow the new permanent-error contract: when store.FindUsersByAccounts returns
only a subset (missing "ghost"), do not expect BulkCreateSubscriptions,
ReconcileMemberCounts or HasOrgRoomMembers to be called; instead call
h.processAddMembers and assert it returns an error that is classified as a
permanent "user not found" failure (use the project's permanent-error predicate
or the model helper you have for checking permanent errors). Keep the mock for
FindUsersByAccounts returning only "bob" and adjust the expectations to verify
no further subscription creation occurs and that the returned error from
processAddMembers is a permanent error.
---
Duplicate comments:
In `@docs/superpowers/specs/2026-04-28-create-room-design.md`:
- Around line 8-13: Update the spec to make the channel-name contract explicit
and consistent: change the summary line describing "channel" so it states that a
client-supplied channel name is required and preserved verbatim (no
auto-generation), and update Scenario C to remove references to an
auto-generated name and instead show the client providing and the system
preserving the given name; ensure any other references in the document that say
"optionally with a name" or imply auto-generation are modified to reflect the
required client-supplied name behavior so the create-room request/response
contract is consistent.
🪄 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: 87bce80e-1501-452c-813f-6c0f241d54a1
📒 Files selected for processing (31)
docs/superpowers/plans/2026-04-28-create-room.mddocs/superpowers/specs/2026-04-28-create-room-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/app.gopkg/model/error.gopkg/model/event.gopkg/model/member.gopkg/model/model_test.gopkg/model/room.gopkg/model/subscription.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
✅ Files skipped from review due to trivial changes (8)
- pkg/pipelines/member_test.go
- pkg/pipelines/member.go
- room-service/mock_store_test.go
- pkg/model/app.go
- room-service/helper.go
- room-service/helper_test.go
- room-worker/mock_store_test.go
- room-worker/store_mongo.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/model/subscription.go
- pkg/model/member.go
- pkg/subject/subject.go
- inbox-worker/integration_test.go
- pkg/model/error.go
- pkg/model/event.go
- pkg/subject/subject_test.go
- room-service/store_mongo.go
- room-worker/store.go
- room-service/integration_test.go
vjauhari-work
force-pushed
the
claude/implement-create-room-ePJUK
branch
from
2babaf5 to
cb745b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
pkg/subject/subject.go (1)
368-388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse
isValidAccountTokenin the other account-bearing parsers.This hardens
ParseRoomCreateSubject, butParseUserRoomSubject,ParseUserRoomSiteSubject, andParseOrgMembersSubjectstill acceptaccounttokens without the shared*/>guard. The boundary rule is still only partially enforced.Based on learnings: “add a single shared guard/validator for any function that accepts an `account` token … reject `*` and `>`.”Patch sketch
func ParseUserRoomSubject(subj string) (account, roomID string, ok bool) { parts := strings.Split(subj, ".") if len(parts) < 5 || parts[0] != "chat" || parts[1] != "user" { return "", "", false } + if !isValidAccountToken(parts[2]) { + return "", "", false + } account = parts[2] // Find "room" token after user position for i := 3; i < len(parts)-1; i++ { if parts[i] == "room" { return account, parts[i+1], true @@ func ParseUserRoomSiteSubject(subj string) (account, roomID, siteID string, ok bool) { parts := strings.Split(subj, ".") if len(parts) < 7 || parts[0] != "chat" || parts[1] != "user" || parts[3] != "room" { return "", "", "", false } + if !isValidAccountToken(parts[2]) { + return "", "", "", false + } return parts[2], parts[4], parts[5], true } @@ func ParseOrgMembersSubject(subj string) (orgID string, ok bool) { parts := strings.Split(subj, ".") if len(parts) != 7 { return "", false } if parts[0] != "chat" || parts[1] != "user" || parts[3] != "request" || parts[4] != "orgs" || parts[6] != "members" { return "", false } + if !isValidAccountToken(parts[2]) { + return "", false + } return parts[5], true }🤖 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 `@pkg/subject/subject.go` around lines 368 - 388, Other account-bearing parsers still allow wildcard account tokens; update ParseUserRoomSubject, ParseUserRoomSiteSubject, and ParseOrgMembersSubject to call the shared isValidAccountToken(account) guard and return false when it fails. Locate each function, identify the parts index containing the account token (the same parts[2] convention used in ParseRoomCreateSubject), and add the check `if !isValidAccountToken(parts[2]) { return "", false }` (or the appropriate zero values) so '*' and '>' are rejected consistently.
🤖 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-04-28-create-room-design.md`:
- Around line 1311-1313: Scenario C's channel subscription entries use
name="bob, org-fx" which contradicts the model rule that channel subscription
Name must equal Room.Name (e.g., "deal team"); update the subscription entries
in the Scenario C block so the channel sub Name fields match Room.Name (change
name="bob, org-fx" and the parallel ian entry to name="deal team" or otherwise
use the Room.Name value), and ensure any sidebarName/roles/siteID remain
unchanged.
- Around line 801-803: Update the idempotency rationale text to remove the
incorrect claim that subscription IDs are deterministic and instead explain that
duplicate-key on bulk insert should be treated as success-on-redelivery because
the unique constraint on (roomId, u.account) prevents duplicate subscriptions;
explicitly note that subscription IDs are UUIDv7 (non-deterministic) and call
out the (roomId, u.account) unique index as the reason duplicate-key indicates a
previously-created row rather than a repeat distinct entity.
- Around line 843-846: The spec text for ReconcileMemberCounts incorrectly
describes two separate $count aggregations; update both occurrences (the
ReconcileMemberCounts description and the add-member call-site description that
references it, and also lines referenced at 996-997) to state that a single
aggregation pipeline uses one $group stage to compute both UserCount and
AppCount (using a conditional accumulator or sum with a bot regex test) and that
the room document is updated atomically; also replace any mention of
ReconcileUserCount with the new single-aggregation contract and ensure the
wording clearly indicates one $group produces both counts.
- Around line 610-623: The dmDedupIndex definition lacks uniqueness — update the
IndexModel Options for dmDedupIndex by adding SetUnique(true) to the options
chain (the same place where SetName(...) and SetPartialFilterExpression(...) are
called) so the partial index on "u.account", "name", "roomType" with roomType in
{"dm","botDM"} is created as a unique index before calling
s.subscriptions.Indexes().CreateOne(ctx, dmDedupIndex).
- Around line 467-472: The capacity check in the canonical pipeline omits the
room creator — when comparing CountNewMembers result (newCount) against
h.maxRoomSize you must account for the owner; update the logic around
CountNewMembers / newCount to include the creator (e.g. treat effectiveCount =
newCount + 1 for the owner) and then compare effectiveCount > h.maxRoomSize to
decide rejection, referencing CountNewMembers, newCount and h.maxRoomSize so the
owner is always counted in capacity.
In `@room-service/handler.go`:
- Around line 283-300: The early-return that returns errEmptyCreateRequest when
allUsers and allOrgs are empty improperly rejects valid creator-only (named)
channel requests; change the condition so you only return errEmptyCreateRequest
if allUsers and allOrgs are empty AND the request has no channel name (the name
field set by classifyAndValidate, e.g. req.Spec.Name or req.Name); leave the
CountNewMembers call and capacity check unchanged so creator-only named channels
proceed and remain consistent with processCreateRoomChannel.
---
Duplicate comments:
In `@pkg/subject/subject.go`:
- Around line 368-388: Other account-bearing parsers still allow wildcard
account tokens; update ParseUserRoomSubject, ParseUserRoomSiteSubject, and
ParseOrgMembersSubject to call the shared isValidAccountToken(account) guard and
return false when it fails. Locate each function, identify the parts index
containing the account token (the same parts[2] convention used in
ParseRoomCreateSubject), and add the check `if !isValidAccountToken(parts[2]) {
return "", false }` (or the appropriate zero values) so '*' and '>' are rejected
consistently.
🪄 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: f8c07f8d-842d-4b22-9fb4-99c7efb5406c
📒 Files selected for processing (31)
docs/superpowers/plans/2026-04-28-create-room.mddocs/superpowers/specs/2026-04-28-create-room-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/app.gopkg/model/error.gopkg/model/event.gopkg/model/member.gopkg/model/model_test.gopkg/model/room.gopkg/model/subscription.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
✅ Files skipped from review due to trivial changes (6)
- pkg/model/error.go
- pkg/model/member.go
- room-service/mock_store_test.go
- pkg/subject/subject_test.go
- pkg/model/event.go
- room-worker/mock_store_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- room-service/main.go
- room-service/store.go
- pkg/model/room.go
- room-worker/store.go
- room-service/store_mongo.go
- pkg/pipelines/member_test.go
- room-service/helper_test.go
- room-service/helper.go
- pkg/pipelines/member.go
- inbox-worker/handler.go
- room-worker/store_mongo.go
- room-service/integration_test.go
- inbox-worker/handler_test.go
- room-service/handler_test.go
- room-worker/integration_test.go
- pkg/model/model_test.go
vjauhari-work
force-pushed
the
claude/implement-create-room-ePJUK
branch
from
cb745b4 to
a43ac3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/superpowers/specs/2026-04-28-create-room-design.md (2)
803-805:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIdempotency rationale at Line 803 is now incorrect (still claims deterministic sub IDs).
This contradicts the UUIDv7 decision described elsewhere in the same spec. Duplicate-key success here should be attributed to the unique
(roomId, u.account)index, not deterministic subscription IDs.Suggested patch
- Duplicate-key on bulk insert → treat as success-on-redelivery (deterministic - IDs mean we wrote these on a previous run; downstream events are NATS-Msg-Id - dedup'd). + Duplicate-key on bulk insert → treat as success-on-redelivery (subscription + IDs are UUIDv7, so redelivery safety comes from the unique compound index on + (roomId, u.account); downstream events are NATS-Msg-Id dedup'd).🤖 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-04-28-create-room-design.md` around lines 803 - 805, The spec text claiming duplicate-key on bulk insert is due to deterministic subscription IDs is incorrect; update the sentence to attribute duplicate-key successes to the unique (roomId, u.account) index and remove or replace any mention of deterministic subscription IDs (and instead reference UUIDv7 where applicable). Specifically, edit the paragraph that mentions "deterministic sub IDs" to explain that duplicate-key indicates a prior successful write for the same (roomId, u.account) key and that downstream deduplication relies on NATS message-id, and ensure any cross-references to UUIDv7 reflect that IDs are not deterministic.
845-848:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ReconcileMemberCountsdescription is inconsistent with the single-aggregation contract.Line 845 and Line 998 still describe two
$countaggregations, which conflicts with the single$groupdesign documented in the PR objectives/post-review fixes.Suggested patch
- Splits the existing single-aggregation count into two: non-bot subs → - UserCount, bot subs (account regex \.bot$) → AppCount. Updates room doc - atomically. Replaces ReconcileUserCount everywhere; add-member's call site - is updated to use ReconcileMemberCounts in this same spec. + Uses a single aggregation (`$group`) to compute both non-bot subs → UserCount + and bot subs (account regex \.bot$) → AppCount, then updates the room doc + atomically. Replaces ReconcileUserCount everywhere; add-member's call site + is updated to use ReconcileMemberCounts in this same spec.-- `ReconcileMemberCounts`: two `$count` aggregations on the `subscriptions` collection (with regex `\.bot$` filter), then a single `rooms.updateOne` with both fields. Atomic from the Room doc's perspective (single update statement). +- `ReconcileMemberCounts`: one aggregation pipeline with a single `$group` on the `subscriptions` collection (with bot/non-bot conditional accumulators, including regex `\.bot$` classification), then one `rooms.updateOne` writing both fields atomically.Also applies to: 998-999
🤖 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-04-28-create-room-design.md` around lines 845 - 848, The ReconcileMemberCounts doc currently describes two separate $count aggregations but the implementation contract uses a single $group with conditional sums; update the description and any example aggregation pipelines in the ReconcileMemberCounts text to show one $group stage that computes UserCount and AppCount via conditional $sum/$cond expressions (e.g. UserCount: {$sum: {$cond: [{$regexMatch: {input: "$account", regex: /\.bot$/}}, 0, 1]}}, AppCount: {$sum: {$cond: [{$regexMatch: {input: "$account", regex: /\.bot$/}}, 1, 0]}}), remove references to two distinct $count aggregations, and ensure both occurrences of the ReconcileMemberCounts documentation reflect this single-$group design.
🧹 Nitpick comments (3)
room-worker/handler.go (1)
948-959: ⚡ Quick winExtract the bot-account predicate into shared code.
This
.bot/p_check is now duplicated in room classification here,room-worker/store_mongo.gomember counting, andpkg/pipelines.GetNewMembersPipelinebot filtering. A shared helper/constant inpkg/would keep classification, filtering, and reconciliation in lockstep and avoid another drift fix later.🤖 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/handler.go` around lines 948 - 959, Extract the ".bot"/"p_" predicate into a single shared helper (e.g., pkg/<name>.IsBotAccount(account string) bool) and replace the inline checks in determineRoomTypeFromPayload (room-worker/handler.go), the member-counting logic in room-worker/store_mongo.go, and the bot filter in pkg/pipelines.GetNewMembersPipeline to call that helper; ensure the helper lives in pkg/ (exported) so all packages import it and update imports/usages accordingly so classification/filtering/reconciliation all use the same IsBotAccount function.room-worker/handler_test.go (1)
2044-2151: ⚡ Quick winAdd the creator-only channel case.
The new channel-create tests all exercise at least one resolved invitee. Please add one case with
ResolvedUsers == nilandResolvedOrgs == nilso the owner-only path is pinned too; that's the boundary case most likely to miss regressions inListNewMembersForNewRoom.As per coding guidelines "Tests must cover: happy path, error paths, edge cases (empty collections, boundary conditions), and invalid input — never write implementation code before its corresponding tests exist".
🤖 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/handler_test.go` around lines 2044 - 2151, Add a new unit test (e.g., TestProcessCreateRoom_Channel_CreatorOnly) that constructs a CreateRoomRequest where ResolvedUsers == nil and ResolvedOrgs == nil (use makeCreateRoomBody and call h.processCreateRoom) and set expectations on mockStore: GetUser(...,"alice") and CreateRoom(...), expect ListNewMembersForNewRoom called with nil slices and return empty list, expect BulkCreateSubscriptions to be called and capture subs and BulkCreateRoomMembers to be called and capture members; then assert only the owner subscription/member is created (one subscription for u_alice with RoleOwner and one RoomMember for u_alice of type RoomMemberIndividual). Ensure you reference processCreateRoom, makeCreateRoomBody, ListNewMembersForNewRoom, BulkCreateSubscriptions and BulkCreateRoomMembers when locating where to add the test.room-service/handler_test.go (1)
2264-2276: 💤 Low value
TestNatsCreateRoom_DMExistsReplycovers onlymodel.ErrorResponseJSON serialization, not thereplyDMExistscall path.The test acknowledges it cannot hook
*nats.Msg.Respondwithout a server. That's a legitimate constraint, but as-is the test will pass even ifreplyDMExistsis completely broken (wrong subject, missing reply, etc.). Consider whether an integration test already coversreplyDMExistsend-to-end — if so, a comment cross-referencing it would clarify intent.🤖 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-service/handler_test.go` around lines 2264 - 2276, The current unit test TestNatsCreateRoom_DMExistsReply only validates model.ErrorResponse JSON and doesn't exercise the replyDMExists path, so update the code/tests by either (A) adding a brief comment in TestNatsCreateRoom_DMExistsReply that references the existing integration test that verifies replyDMExists end-to-end (if such a test exists), or (B) refactor replyDMExists to separate the payload-building logic into a helper function (e.g., buildDMExistsResponse or marshalDMExistsPayload) and unit-test that helper directly from TestNatsCreateRoom_DMExistsReply so the actual bytes and fields are validated; locate replyDMExists and its payload construction in the handler code to implement the refactor and update the test to call the new helper or add the integration test reference comment.
🤖 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-04-28-create-room-design.md`:
- Line 401: There are unlabeled fenced code blocks causing markdownlint MD040
warnings; for each triple-backtick block in the document (the unlabeled ```
blocks around the noted sections), add an appropriate language tag (e.g.,
```text, ```json, or ```go) to the opening fence so the linter recognizes the
block type — pick the language that matches the block content and ensure all
occurrences mentioned (and any other unlabeled fences) are updated consistently.
- Around line 1282-1283: The example in "Scenario C" uses a non-existent
excludeAccount parameter (excludeAccount=alice) causing drift with the defined
signatures CountNewMembers(ctx, orgIDs, directAccounts, roomID) and
ListNewMembersForNewRoom(ctx, orgIDs, accounts); remove the invalid
excludeAccount usage from the examples and instead adjust the narrative/examples
to reflect the real API (e.g., call CountNewMembers(orgs, users, "", roomID) or
document filtering by omitting the account from the accounts/directAccounts list
before calling ListNewMembersForNewRoom), ensuring all occurrences at the
example lines referencing excludeAccount (including the ones at 1282 and 1289)
are updated to match the real function parameters.
In `@room-worker/store_mongo.go`:
- Around line 138-156: The ListNewMembersForNewRoom function should
short-circuit when there are no candidate orgs or accounts (creator-only
channel) like ListNewMembers does: before calling
pipelines.GetNewMembersPipeline, check if len(orgIDs)==0 && len(accounts)==0 and
immediately return nil, nil; this avoids relying on the pipeline's behavior for
empty inputs and ensures deterministic "no invitees" for owner-only room
creation.
---
Duplicate comments:
In `@docs/superpowers/specs/2026-04-28-create-room-design.md`:
- Around line 803-805: The spec text claiming duplicate-key on bulk insert is
due to deterministic subscription IDs is incorrect; update the sentence to
attribute duplicate-key successes to the unique (roomId, u.account) index and
remove or replace any mention of deterministic subscription IDs (and instead
reference UUIDv7 where applicable). Specifically, edit the paragraph that
mentions "deterministic sub IDs" to explain that duplicate-key indicates a prior
successful write for the same (roomId, u.account) key and that downstream
deduplication relies on NATS message-id, and ensure any cross-references to
UUIDv7 reflect that IDs are not deterministic.
- Around line 845-848: The ReconcileMemberCounts doc currently describes two
separate $count aggregations but the implementation contract uses a single
$group with conditional sums; update the description and any example aggregation
pipelines in the ReconcileMemberCounts text to show one $group stage that
computes UserCount and AppCount via conditional $sum/$cond expressions (e.g.
UserCount: {$sum: {$cond: [{$regexMatch: {input: "$account", regex: /\.bot$/}},
0, 1]}}, AppCount: {$sum: {$cond: [{$regexMatch: {input: "$account", regex:
/\.bot$/}}, 1, 0]}}), remove references to two distinct $count aggregations, and
ensure both occurrences of the ReconcileMemberCounts documentation reflect this
single-$group design.
---
Nitpick comments:
In `@room-service/handler_test.go`:
- Around line 2264-2276: The current unit test TestNatsCreateRoom_DMExistsReply
only validates model.ErrorResponse JSON and doesn't exercise the replyDMExists
path, so update the code/tests by either (A) adding a brief comment in
TestNatsCreateRoom_DMExistsReply that references the existing integration test
that verifies replyDMExists end-to-end (if such a test exists), or (B) refactor
replyDMExists to separate the payload-building logic into a helper function
(e.g., buildDMExistsResponse or marshalDMExistsPayload) and unit-test that
helper directly from TestNatsCreateRoom_DMExistsReply so the actual bytes and
fields are validated; locate replyDMExists and its payload construction in the
handler code to implement the refactor and update the test to call the new
helper or add the integration test reference comment.
In `@room-worker/handler_test.go`:
- Around line 2044-2151: Add a new unit test (e.g.,
TestProcessCreateRoom_Channel_CreatorOnly) that constructs a CreateRoomRequest
where ResolvedUsers == nil and ResolvedOrgs == nil (use makeCreateRoomBody and
call h.processCreateRoom) and set expectations on mockStore:
GetUser(...,"alice") and CreateRoom(...), expect ListNewMembersForNewRoom called
with nil slices and return empty list, expect BulkCreateSubscriptions to be
called and capture subs and BulkCreateRoomMembers to be called and capture
members; then assert only the owner subscription/member is created (one
subscription for u_alice with RoleOwner and one RoomMember for u_alice of type
RoomMemberIndividual). Ensure you reference processCreateRoom,
makeCreateRoomBody, ListNewMembersForNewRoom, BulkCreateSubscriptions and
BulkCreateRoomMembers when locating where to add the test.
In `@room-worker/handler.go`:
- Around line 948-959: Extract the ".bot"/"p_" predicate into a single shared
helper (e.g., pkg/<name>.IsBotAccount(account string) bool) and replace the
inline checks in determineRoomTypeFromPayload (room-worker/handler.go), the
member-counting logic in room-worker/store_mongo.go, and the bot filter in
pkg/pipelines.GetNewMembersPipeline to call that helper; ensure the helper lives
in pkg/ (exported) so all packages import it and update imports/usages
accordingly so classification/filtering/reconciliation all use the same
IsBotAccount function.
🪄 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: 899a6dd0-49c8-4704-a8ba-a0ad85036d14
📒 Files selected for processing (31)
docs/superpowers/plans/2026-04-28-create-room.mddocs/superpowers/specs/2026-04-28-create-room-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/integration_test.gopkg/model/app.gopkg/model/error.gopkg/model/event.gopkg/model/member.gopkg/model/model_test.gopkg/model/room.gopkg/model/subscription.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
✅ Files skipped from review due to trivial changes (7)
- pkg/pipelines/member.go
- room-service/store.go
- room-service/mock_store_test.go
- room-worker/mock_store_test.go
- pkg/model/event.go
- pkg/model/subscription.go
- room-service/helper_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/model/error.go
- pkg/model/room.go
- room-service/main.go
- pkg/pipelines/member_test.go
- inbox-worker/handler_test.go
- pkg/subject/subject_test.go
- pkg/subject/subject.go
- inbox-worker/integration_test.go
- inbox-worker/handler.go
- room-service/store_mongo.go
- room-service/integration_test.go
- room-service/helper.go
- room-worker/integration_test.go
- room-service/handler.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/superpowers/specs/2026-04-28-create-room-design.md (1)
800-804:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale idempotency rationale at step 6 — "deterministic IDs" language persists despite being flagged and supposedly fixed.
Lines 802–804 still read "deterministic IDs mean we wrote these on a previous run", but the spec elsewhere (§ Sub-ID and Member-ID generation, the idempotency table at step 6) and the PR description establish that subscription IDs are random UUIDv7. The duplicate-key success condition is the unique
(roomId, u.account)compound index, not deterministic IDs.6. Persist subscriptions: err := h.store.BulkCreateSubscriptions(ctx, subs) - Duplicate-key on bulk insert → treat as success-on-redelivery (deterministic - IDs mean we wrote these on a previous run; downstream events are NATS-Msg-Id - dedup'd). + Duplicate-key on bulk insert → treat as success-on-redelivery. The unique + compound index on (roomId, u.account) ensures the sub already exists for + this user/room pair. Downstream events are NATS-Msg-Id dedup'd.🤖 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-04-28-create-room-design.md` around lines 800 - 804, Update the stale idempotency wording in the "Persist subscriptions" step: replace the phrase claiming "deterministic IDs mean we wrote these on a previous run" with text that explains duplicate-key on BulkCreateSubscriptions (BulkCreateSubscriptions, subs) should be treated as success because subscription uniqueness is enforced by the (roomId, u.account) compound index and subscription IDs are random UUIDv7 (not deterministic); ensure the spec references the compound index as the reason for safe de-duplication and remove any mention of deterministic IDs.
🤖 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-04-28-create-room-design.md`:
- Around line 481-508: The pseudocode step ordering is wrong: move the
deduplication call (h.store.FindDMSubscription / FindDMSubscription) before
calling h.store.GetApp (GetApp) so that the handler returns an existing botDM
(errDMAlreadyExists with existing.RoomID) without invoking
GetApp/Assistant.Enabled; update steps 8b/8d to call FindDMSubscription
immediately after computing req.RoomID (idgen.BuildDMRoomID) and only call
GetApp when no existing subscription is found.
- Around line 1115-1154: subscriptionSidebarName and subscriptionIsSubscribed
only check strings.HasSuffix(account, ".bot") and miss the "p_" webhook-bot
prefix, causing misclassification; add a shared predicate isBotAccount(account
string) bool that returns true for strings.HasSuffix(account, ".bot") OR
strings.HasPrefix(account, "p_"), then use isBotAccount in
subscriptionSidebarName and subscriptionIsSubscribed (and any other inbox-worker
helpers like the earlier subscriptionName logic if present) in place of the
current strings.HasSuffix checks so webhook bots on remote sites are treated the
same as home-site classification.
- Around line 467-476: Step 7e must add the creator-only guard: after calling
h.store.CountNewMembers(ctx, allOrgs, allUsers, "", requesterAccount) check for
error and if newCount == 0 return errEmptyCreateRequest (the same behavior
asserted by TestHandleCreateRoom_Channel_ChannelRefsExpandToCreatorOnly) before
performing the upper-bound check (newCount + 1 > h.maxRoomSize); update the
handler code path that currently generates req.RoomID (idgen.GenerateID) to only
proceed when CountNewMembers > 0 and include the error handling for
h.store.CountNewMembers failures.
---
Duplicate comments:
In `@docs/superpowers/specs/2026-04-28-create-room-design.md`:
- Around line 800-804: Update the stale idempotency wording in the "Persist
subscriptions" step: replace the phrase claiming "deterministic IDs mean we
wrote these on a previous run" with text that explains duplicate-key on
BulkCreateSubscriptions (BulkCreateSubscriptions, subs) should be treated as
success because subscription uniqueness is enforced by the (roomId, u.account)
compound index and subscription IDs are random UUIDv7 (not deterministic);
ensure the spec references the compound index as the reason for safe
de-duplication and remove any mention of deterministic IDs.
🪄 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: 00239fc4-bae2-455e-a915-331b63a2e8eb
📒 Files selected for processing (3)
docs/superpowers/specs/2026-04-28-create-room-design.mdroom-service/handler.goroom-service/handler_test.go
✅ Files skipped from review due to trivial changes (1)
- room-service/handler.go
mliu33
reviewed
May 5, 2026
Collaborator
mliu33
left a comment
mliu33
left a comment
There was a problem hiding this comment.
Outstanding work! Thanks! Just some questions and suggestions
|
|
||
| req.Users = stripAccount(dedup(req.Users), requesterAccount) | ||
| roomType := determineRoomType(req) | ||
|
|
Collaborator
There was a problem hiding this comment.
I think we can just check if req.Users has length 1 and whether it's self-DM at this line (after req.Users = stripAccount(...)), so we only need to run one dedupe for loop. And we can remove the self-DM check above
Collaborator
Author
There was a problem hiding this comment.
Done in commit 89936dc (Task 12 of the v2 cleanups): classifyAndValidate now does a single dedup + stripAccount pass, and the self-DM detection is len(deduped) == 1 && len(req.Users) == 0 (post-strip empty + pre-strip singleton ⇒ that singleton was the requester). One dedup loop, no second pass.
Generated by Claude Code
| if timestamp <= 0 { | ||
| slog.Error("restricted history with missing timestamp, emitting nil", "roomID", roomID, "mode", mode) | ||
| slog.Error("restricted history with missing timestamp, emitting nil", "roomID", roomID, "mode", history.Mode) | ||
| return nil |
Collaborator
There was a problem hiding this comment.
We should return current timestamp instead of nil if request timestamp is missing, which should not happen. But for extra protection, it's better to set to current time instead of nil for restricted history mode for the user
Collaborator
Author
There was a problem hiding this comment.
Already in place (and re-verified during the v2 cleanup). processCreateRoom defaults req.Timestamp = time.Now().UTC().UnixMilli() when req.Timestamp <= 0, so acceptedAt = time.UnixMilli(req.Timestamp).UTC() is always a real time, never a nil/epoch fallback. Same pattern is used by processAddMembers etc.
Generated by Claude Code
vjauhari-work
force-pushed
the
claude/implement-create-room-ePJUK
branch
from
e06d50a to
c2e09fe
Compare
vjauhari-work
force-pushed
the
claude/implement-create-room-ePJUK
branch
from
69930d1 to
3aef95b
Compare
Adds the implementation spec for the create-room feature. Clients can
create dm, botDM, and channel rooms via a new NATS request/reply
endpoint chat.user.{account}.request.room.{siteID}.create. Room and
room_members documents live only on the requester's site;
subscriptions are replicated cross-site via a new room_created outbox
event consumed by inbox-worker. The flow mirrors add-member end to end
(room-service validates → ROOMS canonical → room-worker writes Mongo
state, fans out subscription.update events, publishes channel-only
sys-messages, emits per-site outbox → inbox-worker creates remote
subs).
Spec covers: scope (in/out), NATS subjects + ROOMS canonical
operation, data models (RoomTypeBotDM, Room.AppCount, Subscription
fields Name/RoomType/SidebarName/IsSubscribed, App/AppAssistant/
AppSponsor, CreateRoomRequest, RoomCreatedOutbox, ErrorResponse.RoomID,
AsyncJobResult Operation values), reply contract (synchronous
{accepted, roomId, roomType} + async-job result on
subject.UserResponse), room-service request handling pipeline (subject
parse, X-Request-ID guard, requester strip, room-type determination,
DM/botDM dedup via subscriptions.findOne({u.account, name, roomType}),
channel-branch validation with channelRef expansion + capacity check),
room-worker handler (Room insert with idempotent matching-existing on
duplicate-key, three subs-construction branches with deterministic IDs
via MessageIDFromRequestID, ReconcileMemberCounts split into
UserCount/AppCount, subscription.update fan-out for every sub,
channel-only sys-messages with deterministic Nats-Msg-Id, cross-site
outbox per remote site keyed by User.SiteID), inbox-worker handler
(handleRoomCreated builds remote-side subs only — never Room or
room_members), four cross-site walk-throughs (single-site channel,
cross-site DM, cross-site channel, single-site botDM), error handling
with permanent-vs-retryable dispatcher pattern, and the testing
strategy. Add-member retrofit lands in the same spec: drop payload
RequestID (header-only per PR #131), deterministic sub IDs,
Subscription.Name/RoomType population, MemberAddEvent.RoomName
extension for cross-site naming, AsyncJobResult on completion. Mongo
indices: {assistant.name} on apps and partial-filter
{u.account, name, roomType} on subscriptions for the DM-dedup query.
Hard prerequisite: PR #131 idgen rework merged.
https://claude.ai/code/session_01Sb9n638Nv6onNKNjUcyUxz
Adds the full TDD implementation plan for the create-room feature.
9 phases, 44 tasks, every task in red→green→commit form with
complete test bodies and final code shapes inline (no placeholders,
no "TODO: implement"). Tasks scoped to 2-5 minutes each.
Phase 1: pkg/model foundation — RoomTypeBotDM, Room.AppCount,
Subscription new fields, App/AppAssistant/AppSponsor,
CreateRoomRequest, MemberAddEvent.RoomName, RoomCreatedOutbox,
ErrorResponse.RoomID, AsyncJobOp constants. Round-trip tests for
every new field, including JSON omitempty assertions.
Phase 2: pkg/subject (RoomCreate / RoomCreateWildcard /
RoomCanonicalOperation extractor / ParseRoomCreateSubject) and
pkg/pipelines (empty-roomID branch in GetNewMembersPipeline that
drops the subscriptions $lookup when the room doesn't exist).
Phase 3: ReconcileMemberCounts split — interface rename, mockgen
regen, Mongo impl with separate user/bot regex CountDocuments and
atomic update; integration test asserting bot/non-bot split.
Phase 4: add-member retrofit (lands first to validate the new
infrastructure) — mandatory X-Request-ID, deterministic sub IDs,
Subscription.Name/RoomType population, MemberAddEvent.RoomName
outbox propagation, AsyncJobResult on completion, inbox-worker
handleMemberAdded counterpart.
Phase 5: room-service create-room — sentinels, dmExistsError typed
wrapper, helpers (stripAccount, truncateRunes, composeAutoName),
sanitizeError extension, store interface (GetUser, GetApp,
FindDMSubscription) + Mongo impl, EnsureIndexes ({assistant.name}
+ partial-filter compound on subscriptions), handleCreateRoom
validation pipeline split across DM/botDM and channel branches,
natsCreateRoom adapter with replyDMExists special path, main.go
wildcard subscription.
Phase 6: room-worker create-room — store extensions (CreateRoom,
GetUser, ListNewMembersForNewRoom), helpers (composeName, newSub,
resolveRoomName, createdByForType), processCreateRoom skeleton
with three branch dispatcher, DM/botDM/channel branch
implementations with deterministic sub IDs, idempotent
matching-existing on Room insert, finishCreateRoom
(subscription.update fan-out, channel-only sys-messages with
deterministic IDs, cross-site outbox keyed by User.SiteID,
AsyncJobResult), main.go dispatcher wiring with permanent-vs-
retryable error handling.
Phase 7: inbox-worker create-room — rolesForType,
subscriptionName/SidebarName/IsSubscribed helpers covering all
three room types and the bot-vs-human asymmetry in botDM,
handleRoomCreated handler with X-Request-ID guard +
empty-Accounts no-op, dispatcher routing for the new
"room_created" event type.
Phase 8: integration tests — room-service end-to-end create-channel
+ DM-dedup; room-worker channel/DM/idempotent-redelivery;
inbox-worker remote-sub persistence asserting NO Room mirror
written.
Phase 9: verification (full lint + unit + integration sweep with
grep tripwires for stale references) and manual smoke test
against docker-compose covering create-channel, botDM with
isSubscribed flag, and DM-dedup.
Self-review confirms every spec section maps to a task, all
type/signature names are consistent across phases, and no
placeholders remain.
Hard prerequisite: PR #131 idgen rework merged.
https://claude.ai/code/session_01Sb9n638Nv6onNKNjUcyUxz
…history fallback - Remove PR #131 prerequisite note (already merged) - Request IDs described as UUID (any version, with hyphens) not "32-char hex" - Subscription and RoomMember IDs switch from MessageIDFromRequestID to GenerateUUIDv7() per the established convention for these collections - Idempotency for BulkCreateSubscriptions / BulkCreateRoomMembers now relies on compound unique indices (roomId, u.account) and (roomId, member.id) rather than deterministic _id values; EnsureIndexes section updated - Add-member retrofit test for history config "none" with missing timestamp: fall back to time.Now() instead of leaving HistorySharedSince nil or erroring - Task 14b added to plan for the history timestamp fallback - Task 13 test rewritten to assert 32-char UUIDv7 format instead of deterministic MessageIDFromRequestID output https://claude.ai/code/session_01W5J2f5xbShC9xv2wDghXua
…ec and plan
- Migrate AsyncJobResult from {Job, Success} to {Operation, Status, RoomID(omitempty)}
- Add all five AsyncJobOp* constants (room.create, room.member.add, room.member.remove,
room.member.remove_org, room.member.role_update) to spec and plan Task 6
- Expand Task 6 test to assert all constants + struct shape migration
- Note in spec that frontend has no consumer yet, so no wire-compat concern
- Fix DM/botDM Room._id description: User.ID is 17-char base62 (external auth),
so BuildDMRoomID produces 34-char sorted concat with no separator
- Replace all "u_alice|u_bob" / "u_alice|u_wbot" pipe-format examples with
correct "u_aliceu_bob" / "u_aliceu_wbot" (BuildDMRoomID has no separator)
https://claude.ai/code/session_01Yc15Zegeuj21DSXMUdknp7
- Fix 36 request-ID test fixtures from 32-char hex to 36-char hyphenated UUIDv7 (CLAUDE.md says 32-char form is reserved for Mongo _id) - Replace AddedUsersCount approximation with the exact post-expansion count (len(allUsers)-1) plumbed through publishChannelSysMessages - Add "Review Adjustments — Apply Throughout" section consolidating cross-cutting rules from the simplify review: reuse h.publish for all JetStream sends, extract publishOutbox helper, refactor not redefine publishAsyncJobResult, promote duplicated helpers to pkg/, add status/op/sysmessage-type constants, typed CreateRoomReply, bulk-write helpers on Store interface, parallelize outbox and subscription.update fan-outs, skip ReconcileMemberCounts for DM, no time.Sleep grep in Task 43 verification
- Sub/member IDs: replace MessageIDFromRequestID("sub:/member:") with
GenerateUUIDv7() throughout Tasks 13,14,33,34,39. The spec's detailed
section (line 344) says 32-char hex; the summary bullet (line 52) was
wrong. Idempotency relies on compound unique indexes, not deterministic
IDs. Test assertion updated to assert.True(idgen.IsValidUUIDv7(id)).
- Integration test reqIDs: GenerateUUIDv7() -> GenerateRequestID() (the
former returns 32-char no-hyphen which fails IsValidUUID in the handler)
- Review Adjustments rule 1: clarify request IDs are any-version
hyphenated UUIDs; GenerateRequestID() is the right call in tests
- Review Adjustments rule 16: document the spec contradiction and
confirm GenerateUUIDv7() is the correct choice for sub/member IDs
ID conventions: - Fix smoke-test request ID: was 32-char no-hyphen, now 36-char hyphenated - Fix integration test sub fixture: "sub_alice" -> idgen.GenerateUUIDv7() - len(allUsers)-1 -> len(subs)-1 for AddedUsersCount (matches spec) Code quality: - publishCanonical now wraps h.publish; outbox publish uses h.publish (eliminates direct h.js.PublishMsg calls in code blocks) - map[string]string reply -> model.CreateRoomReply typed struct in publishCreateRoom and all test deserialization sites - Remove all // NEW and // OLD inline comments from code blocks - Remove finishCreateRoom TODO comment; use inline note instead - Outbox envelope type: "room_created" -> model.MessageTypeRoomCreated - Async-job Status: "ok" -> model.AsyncJobStatusOK - inbox-worker dispatcher: case "room_created" -> model.MessageTypeRoomCreated - Add idgen.IsValidUUID() format validation to Task 24 request-ID check Spec alignment: - Task 23 EnsureIndexes: add missing subscriptions (roomId, u.account) unique index and room_members (roomId, member.id) unique index — required for JetStream redelivery idempotency per spec §5.3 Review Adjustments: - Rule 2: reconcile with publishCanonical being the correct thin wrapper - Rule 12: canonize len(subs)-1 to match spec wording
Added AppCount field to Room struct with json and bson tags. Verified with TestRoomBotDMRoundtrip to ensure JSON serialization works correctly. https://claude.ai/code/session_01TQCiKLv9Ec7s5ZSctD72qn
Adds the canonical CreateRoomRequest struct to pkg/model/member.go (Name/Users/Orgs/Channels + RoomID/RequesterID/RequesterAccount/AppName/Timestamp), removes the legacy shape from pkg/model/room.go, and updates room-service/handler.go to use the new field names (RequesterID, RequesterAccount, Users, h.siteID). Adds three new round-trip tests in pkg/model/model_test.go. https://claude.ai/code/session_01TQCiKLv9Ec7s5ZSctD72qn
…box; ErrorResponse.RoomID - Replace AsyncJobResult.Job/Success with Operation/Status string fields; add op constants - Add RoomCreatedOutbox cross-site payload for room creation federation - Add ErrorResponse.RoomID (omitempty) for DM-already-exists responses - Update room-worker publishAsyncJobResult signature and 3 call sites to use op constants - Add 5 new model round-trip/shape tests https://claude.ai/code/session_01TQCiKLv9Ec7s5ZSctD72qn
…up order, bot predicate - Step 7e: document the newCount == 0 reject (creator-only channels). - Step 8: dedup before GetApp so existing botDM is returned even if bot was disabled later (matches handler order in code). - inbox-worker pseudo helpers: use shared isBot predicate covering both ".bot" suffix and "p_" prefix (mirrors home-site classification).
Spec deltas captured by the 2026-05-05 update marker:
- Drop Subscription.SidebarName. Subscription.Name now carries the
counterpart account for DM/botDM (Room.Name empty); channel rules
unchanged. RoomCreatedOutbox drops RequesterEngName,
RequesterChineseName, AppName — frontend resolves display names
from the locally-replicated users/apps collections.
- Room.CreatedBy is the requester's User.ID for every room type
(no more empty-CreatedBy on DM/botDM).
- Room.Name is empty for DM/botDM (no per-subscriber display name on
the room doc).
- room-service: skip EngName/ChineseName check on DM/botDM
counterpart (apps don't populate those fields). DM dedup runs
before GetApp so an existing botDM is returned even if the bot
was disabled later.
- room-service: self-DM detection folded into the post-strip length
check; one dedup pass.
- room-service.helper.sanitizeError: two pass-through case groups
merged into one branch.
- room-service MongoStore: explicit `apps *mongo.Collection` field;
drop the unused `*mongo.Database` retention.
- room-worker: single `acceptedAt` derivation at top of
processCreateRoom with `req.Timestamp > 0` fallback to time.Now();
reused across all branches.
- room-worker channel branch: owner sub built in the same loop as
member subs; no trailing append.
- inbox-worker: subscriptionSidebarName helper removed alongside the
field.
- Scenarios A/B/C/D updated to reflect the new field shapes.
- Test plan updated; new asserts cover Name, IsSubscribed, and
CreatedBy for each room type.
Follow-ups added:
- Sync server-to-server DM endpoint
(chat.server.request.room.{siteID}.create.dm) deferred to a
separate PR. Notes the planned shape and the small refactor
needed (extract the room-create + collision prelude into a
shared helper) so the sync handler can reuse
processCreateRoomDM/BotDM.
- DM-creation cross-site tiebreak recorded as a known gap.
…or botDM counterpart botDMs counterpart user is an app/bot whose users-collection record typically has empty EngName/ChineseName; the existing GetApp + Assistant.Enabled check is the right validation for that case.
Covers the 17 round-3 review comments (mliu) plus the v2 spec deltas: - pkg/model field drops (SidebarName, RequesterEngName, RequesterChineseName, AppName) - room-service validation refactors (botDM counterpart name check, self-DM consolidation, sanitizeError merge, MongoStore apps field) - room-worker code structure (CreatedBy=requester always, Room.Name="" for DM/botDM, single channel-sub loop, drop composeName helpers) - inbox-worker sidebar helper removal - All accompanying test updates Sync server-to-server DM endpoint deferred to a separate PR per the spec follow-ups section.
Spec: new add-member retrofit item #6 — direct bots in req.Users return errBotInChannel; bots from channel-ref expansion are silently filterBots'd. Mirrors create-channel; today the pkg/pipelines regex silently drops them at the DB so the outcome is correct, but the client gets 'accepted' on a request that explicitly listed a bot. Plan: new Task 16 covering both halves with TDD steps.
- pkg/model: drop Subscription.SidebarName, RoomCreatedOutbox.{RequesterEngName, RequesterChineseName, AppName}, CreateRoomRequest.AppName.
- room-worker: drop composeName/composeNameOrAccount helpers; newSub no longer takes sidebarName; DM/botDM/channel branches use raw counterpart accounts; Room.CreatedBy = requester.ID for all room types; Room.Name = "" for DM/botDM; channel sub building consolidated into a single loop including the owner; outbox publish drops dropped fields.
- inbox-worker: drop subscriptionSidebarName and composeName helpers; sub construction drops the SidebarName field write.
- Tests updated across pkg/model, room-worker (handler + integration), and inbox-worker (handler + integration).
…13, 14)
- Skip EngName/ChineseName check on botDM counterpart (apps don't
populate those fields in the users collection); add the
TestHandleCreateRoom_BotDM_AppCounterpartNoNameFields test.
- classifyAndValidate: consolidate self-DM detection into one
dedup/strip pass with a length comparison; drop the pre-strip
pass.
- helper.sanitizeError: merge the two err.Error() pass-through case
groups into a single branch (errNotRoomMember stays separate as
it returns the bare message).
- MongoStore: replace the unused *mongo.Database field with an
explicit apps *mongo.Collection field; bind in NewMongoStore;
rewrite the two s.db.Collection("apps") call sites.
- Direct bots in req.Users return errBotInChannel (hard error, matches classifyAndValidate behavior in create-channel). - Bots from expandChannelRefs are silently filtered before the merge (matches handleCreateRoomChannel). Today the pkg/pipelines regex drops bots at the DB layer so the outcome is correct, but a client that explicitly listed a bot gets `accepted`. This makes the rejection explicit at the room-service boundary. Tests: TestHandleAddMembers_RejectsDirectBot and TestHandleAddMembers_SilentlyFiltersBotsFromChannelRefs.
- helper: errBotInChannel message no longer says "during creation" (the sentinel is now returned from handleAddMembers too). - handler_test: rename two new add-member bot tests to match the TestHandler_AddMembers_* convention used elsewhere in the file. - handler: extend classifyAndValidate doc comment to mention the dedup/strip ordering after Task 12's refactor.
TestProcessCreateRoomDMPersistsTwoSubsAndZeroMembers asserted room.CreatedBy was empty for DMs, matching the pre-v2 behavior. Block 1's Task 6 made CreatedBy = requester.ID for every room type; the integration assertion now matches.
- room-worker: assert RoomCreatedOutbox emission per remote site for channel and DM create-room paths. - room-worker: assert MemberAddEvent outbox emission per remote site for add-member. - inbox-worker: DM mirror of TestHandleRoomCreatedPersistsRemoteSubs asserting Subscription.Name=counterpart-account and Room.Name="". Closes the home-side outbox-emission coverage gap that was the weakest link in the cross-site contract.
vjauhari-work
force-pushed
the
claude/implement-create-room-ePJUK
branch
from
3aef95b to
0132d31
Compare
mliu33
approved these changes
May 6, 2026
| if history.Mode != model.HistoryModeNone { | ||
| return nil | ||
| } | ||
| if history.SharedSince != nil && *history.SharedSince > 0 { |
Collaborator
There was a problem hiding this comment.
I think this is actually not correct. HistoryConfig should never have this sharedSince field because client frontend should never pass the timestamp directly, and the backend server should never use the one specified by the caller.
Could you fix this in a new PR ?
Collaborator
|
Please have a separate PR to fix the history config and just use acceptedAt for historySharedSince timestamp in room-worker. Thanks~ |
Collaborator
|
Excellent work! Thanks! |
vjauhari-work
pushed a commit
that referenced
this pull request
May 7, 2026
…nfig.SharedSince removal
Two related-but-distinct cleanups batched into a single PR (both
touch pkg/model and room-service):
Part 1 — Sync server-to-server DM creation
New NATS subject chat.server.request.room.{siteID}.create.dm with
server credentials. Reply contains the full Room and a `created`
flag. Internally inlines the existing async create-DM/botDM
validation, persistence, subscription.update fan-out, and
cross-site outbox emission — no JetStream hop, no AsyncJobResult,
no room-worker involvement. Reuses room-worker logic via a new
pkg/createroom package (CreateRoomDoc, BuildDMSubs, BuildBotDMSubs).
Part 2 — Remove HistoryConfig.SharedSince
Per reviewer feedback on PR #142. Backend never reads a
client-supplied timestamp for history restriction; it always uses
acceptedAt (the room-service request-acceptance time stamped at
request entry). historySharedSincePtr signature reduces to
(mode, acceptedAt) -> *int64 with no fallback chain.
This was referenced May 8, 2026
Merged
This was referenced May 20, 2026
This was referenced Jun 1, 2026
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
Implements the createRoom feature spanning room-service (sync entry), room-worker (async finalize + cross-site outbox), and inbox-worker (remote sub creation). Supports three room types — DM, botDM, and channel — with idempotency, DM dedup, system messages, async-job results, and cross-site federation.
Spec:
docs/superpowers/specs/2026-04-28-create-room-design.mdPlan:
docs/superpowers/plans/2026-04-28-create-room.mdWhat's in the PR
room-service (sync NATS request/reply)
handleCreateRoomwithclassifyAndValidatehoisting all validation up frontidgen.BuildDMRoomID,appscollection lookup gated onAssistant.Enabled, DM dedup via partial unique index on{u.account, name, roomType $in {dm,botDM}}returning{error:"dm already exists", roomId}on hitexpandChannelRefswith per-refMEMBER_LIST_TIMEOUTand typedchannelExpandTimeoutError{SiteID, RoomID}, silentfilterBotson expansion (documented in spec), capacity check via empty-roomID variant ofCountNewMembers{status:"accepted", roomId, roomType}room-worker (async JetStream finalize)
processCreateRoomdispatches to DM/botDM/channel branches →finishCreateRoomepilogue (ReconcileMemberCounts, subscription.update fan-out, sys-messages, outbox, async-job result)permanentErrortype) Ack instead of Nak — error event reaches the requester via thedefer publishAsyncJobResultwithout infinite redelivery$groupaggregation reconcilesUserCount+AppCountinbox-worker (cross-site receipt)
handleRoomCreatedmaterializes remote-side subs from outbox payload, mirroring home-site rules forSubscription.Name/SidebarName/IsSubscribedacross DM, botDM, and channelShared (
pkg/)idgen: UUIDv7 generator for subs/members; deterministic DM room IDsubject:RoomCreate/RoomCreateWildcard/RoomCanonicalOperation/ParseRoomCreateSubjectmodel:CreateRoomRequest,CreateRoomReply,RoomCreatedOutbox,App/AppAssistant/AppSponsor,Subscription.Name/RoomType/SidebarName/IsSubscribed,Room.AppCount,MemberAddEvent.RoomName,ErrorResponse.RoomID,AsyncJobResultmigrated toOperation/Statuspipelines: empty-roomID branch inGetNewMembersPipelinePost-review fixes (commit
4251143)errPermanent(no redelivery storm)expandChannelRefswith site+roomId in the errorpermanentErrortype replaces fragile%w:%wwrap and string-suffix sanitizationTest plan
make lint— 0 issuesmake test— all packages green (race detector enabled)make test-integration SERVICE=room-service— 16.5smake test-integration SERVICE=room-worker— 10.5smake test-integration SERVICE=inbox-worker— 4.3sTestProcessCreateRoom_RoomIDCollisionMismatchType_ReturnsPermanent,TestSanitizeAsyncJobError_*,TestHandleRoomCreatedBotDMBuildsRemoteBotSub, twoexpandChannelRefsdeadline-exceeded subtestshttps://claude.ai/code/session_01KqM2GGiPtcaKqsEdv9fo61
Generated by Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests