Fix member-add bug + dept-aware org matching + Cassandra cleanups#212
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:
📝 WalkthroughWalkthroughImplements dept-aware org matching and org→individual membership upgrades: new Mongo aggregation pipelines and store APIs, room-service request-time validators, room-worker candidate-driven add-members/backfill flow, Cassandra schema & Message model cleanup, frontend DM dedup shortcut, key-fanout refactor, and supporting tests/docs. ChangesOrg-to-individual membership and department-aware support
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ 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: 7
🧹 Nitpick comments (1)
room-worker/store.go (1)
86-88: ⚡ Quick winUpdate stale method-reference comment to the new contract.
The comment still says “ListNewMembers”, but this interface now uses
ListAddMemberCandidates. Keeping this stale name will mislead future maintenance.Suggested diff
- // ListNewMembersForNewRoom is the empty-roomID variant of - // ListNewMembers — same dedup + bot filter, no "already-subscribed" + // ListNewMembersForNewRoom is the empty-roomID variant of + // ListAddMemberCandidates candidate resolution — same dedup + bot filter, no "already-subscribed"🤖 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/store.go` around lines 86 - 88, The method comment for ListNewMembersForNewRoom is stale: it references "ListNewMembers" but the interface now uses ListAddMemberCandidates; update the comment to mention ListAddMemberCandidates as the related contract and adjust the description to match the current behavior (dedup + bot filter, no already-subscribed pruning for non-existent rooms, and note excludeAccount semantics) so future readers are not misled by the old name.
🤖 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/plans/2026-05-19-member-add-improvements-plan.md`:
- Around line 1581-1583: The plan snippet still uses the stale key "_orgRaw"
(e.g., in the BSON projection) and mentions display._orgRaw; update the snippet
to use the new field name "orgRaw" instead (replace "_orgRaw" with "orgRaw") and
update any documentation references from display._orgRaw to display.orgRaw so
the plan matches the PR hardening notes and current schema.
- Around line 136-139: The unlabeled fenced code blocks containing command lines
like "make build SERVICE=room-worker" and "make build SERVICE=room-service"
should be updated to include a language identifier (e.g., ```sh or ```bash) so
markdownlint MD040 warnings go away and snippets are easier to scan; scan the
file for other unlabeled fences (notably ones showing shell, Go, or JSON
examples) and add the appropriate labels (```sh/```bash for shell commands,
```go for Go code, ```json for JSON) to each fenced block.
In
`@docs/superpowers/specs/2026-05-19-org-to-individual-membership-upgrade-design.md`:
- Around line 38-44: Several fenced code blocks in the spec are missing language
annotations which triggers markdownlint MD040; update each unlabeled fenced
block that contains Go pipeline signatures (notably the blocks with
GetCapacityCheckPipeline and GetAddMemberCandidatesPipeline) and the other
examples referenced (lines ~60-72, ~172-187, ~195-200) by adding the appropriate
language identifier (e.g., ```go for Go snippets, ```json or ```shell where
applicable) so the fences are annotated and linting/readability are fixed.
In `@Makefile`:
- Around line 118-121: Update the Makefile lines that invoke docker compose so
COMPOSE_BAKE is enabled: change the environment variable COMPOSE_BAKE from false
to true in both occurrences that prefix the docker compose commands (the lines
setting "COMPOSE_BAKE=false COMPOSE_PARALLEL_LIMIT=1 docker compose ..."),
keeping COMPOSE_PARALLEL_LIMIT=1 unchanged so builds remain serialized while
enabling Buildx Bake for reproducible builds.
In `@pkg/displayfmt/combine_test.go`:
- Around line 1-9: The test file currently declares package displayfmt_test
which violates the repo rule to use the same package for *_test.go files; change
the package declaration in combine_test.go from displayfmt_test to displayfmt so
the tests run in-package and can access unexported identifiers, and then update
any test references/imports (remove the local import of
github.com/hmchangw/chat/pkg/displayfmt if it's only used because of the _test
package) to match the new package.
In `@room-service/integration_test.go`:
- Around line 895-903: The test "matches dept users when orgId equals deptId
without parent sect match" is accidentally using SectID == "dept-x" for
dept-scoped users so the test can pass via sect match; update the test data
created with insertUser (model.User instances) so that the user meant to
exercise dept-only matching has a non-matching SectID (e.g., set SectID to
something other than the DeptID like "sect-other" or empty) while keeping DeptID
== "dept-x", and ensure the other user in the same test still reflects the
intended non-dept user; apply the same change to the similar test case covering
lines ~949-956 to remove SectID == DeptID coupling.
In `@room-worker/handler.go`:
- Line 1657: Replace the bare return of reconcileErr with a wrapped error that
preserves context and the original error (e.g., use fmt.Errorf("reconcile sync
failed: %w", reconcileErr)); locate the return that currently reads "return nil,
reconcileErr" and change it to return nil, fmt.Errorf("reconcile sync failed:
%w", reconcileErr) (ensure fmt is imported or use errors.Wrap-equivalent), so
call-site context is preserved in logs/traces.
---
Nitpick comments:
In `@room-worker/store.go`:
- Around line 86-88: The method comment for ListNewMembersForNewRoom is stale:
it references "ListNewMembers" but the interface now uses
ListAddMemberCandidates; update the comment to mention ListAddMemberCandidates
as the related contract and adjust the description to match the current behavior
(dedup + bot filter, no already-subscribed pruning for non-existent rooms, and
note excludeAccount semantics) so future readers are not misled by the old name.
🪄 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: 67166e5b-f66f-4b79-98db-5bc3dd4c5c2a
📒 Files selected for processing (48)
Makefilechat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.jsxchat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.test.jsxdocker-local/cassandra/init/10-table-messages_by_room.cqldocker-local/cassandra/init/11-table-thread_messages_by_room.cqldocker-local/cassandra/init/12-table-pinned_messages_by_room.cqldocker-local/cassandra/init/13-table-messages_by_id.cqldocs/cassandra_message_model.mddocs/client-api.mddocs/superpowers/plans/2026-05-19-member-add-improvements-plan.mddocs/superpowers/specs/2026-05-19-org-to-individual-membership-upgrade-design.mdhistory-service/docker-local/docker-compose.ymlhistory-service/internal/cassrepo/integration_test.gohistory-service/internal/cassrepo/messages_by_id_integration_test.gohistory-service/internal/cassrepo/messages_by_room.gohistory-service/internal/cassrepo/thread_messages.gohistory-service/internal/cassrepo/thread_messages_integration_test.gohistory-service/internal/service/integration_test.goinbox-worker/handler_test.gopkg/displayfmt/combine.gopkg/displayfmt/combine_test.gopkg/model/cassandra/message.gopkg/model/cassandra/message_test.gopkg/model/model_test.gopkg/model/room.gopkg/model/user.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/roomkeymetrics/metrics.gopkg/roomkeystore/integration_test.gopkg/roomsubcache/integration_test.gopkg/testutil/testimages/testimages.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.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.goroom-worker/sysmsg.goroom-worker/sysmsg_test.go
💤 Files with no reviewable changes (13)
- docker-local/cassandra/init/10-table-messages_by_room.cql
- pkg/model/room.go
- history-service/internal/cassrepo/integration_test.go
- docker-local/cassandra/init/11-table-thread_messages_by_room.cql
- pkg/model/cassandra/message_test.go
- chat-frontend/src/api/types.ts
- pkg/model/cassandra/message.go
- docs/cassandra_message_model.md
- docker-local/cassandra/init/12-table-pinned_messages_by_room.cql
- docker-local/cassandra/init/13-table-messages_by_id.cql
- chat-frontend/src/api/fetchSidebarBuckets/index.ts
- history-service/docker-local/docker-compose.yml
- pkg/roomkeymetrics/metrics.go
vjauhari-work
force-pushed
the
claude/fix-member-subscription-bug-QZhjc
branch
from
6864cb3 to
3df2b19
Compare
vjauhari-work
added a commit
that referenced
this pull request
May 20, 2026
- pkg/displayfmt/combine_test.go: switch to in-package tests (`package displayfmt`) per CLAUDE.md §4 rule; drop the now-unused local import. - room-worker/handler.go: wrap bare `return nil, reconcileErr` in processSyncCreateDM's dup-key reconcile branch with sync-DM context (CLAUDE.md §3 "Never return bare err"). - room-service/integration_test.go: make two dept-only test cases actually dept-only by setting SectID to a non-matching value (was `dept-x`, so the tests would have passed via sectId match alone — defeating the intent of "dept-only branch is doing its job"). - room-worker/store.go: refresh stale doc on ListNewMembersForNewRoom — refer to ListAddMemberCandidates (the current contract) instead of the removed ListNewMembers. - docs/superpowers: rename stale `_orgRaw` → `orgRaw` in both the plan snippet and the spec narrative to match the renamed BSON tag; add language identifiers (```sh) to the four shell-command fenced blocks the reviewer called out in the plan. Skipped intentionally: - Makefile COMPOSE_BAKE finding: the user explicitly asked to REMOVE COMPOSE_BAKE/COMPOSE_PARALLEL_LIMIT (commit 3df2b19); bot's suggestion to set COMPOSE_BAKE=true contradicts that directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full-review |
|
✅ Actions performedFull review triggered. |
vjauhari-work
added a commit
that referenced
this pull request
May 21, 2026
- pkg/displayfmt/combine_test.go: switch to in-package tests (`package displayfmt`) per CLAUDE.md §4 rule; drop the now-unused local import. - room-worker/handler.go: wrap bare `return nil, reconcileErr` in processSyncCreateDM's dup-key reconcile branch with sync-DM context (CLAUDE.md §3 "Never return bare err"). - room-service/integration_test.go: make two dept-only test cases actually dept-only by setting SectID to a non-matching value (was `dept-x`, so the tests would have passed via sectId match alone — defeating the intent of "dept-only branch is doing its job"). - room-worker/store.go: refresh stale doc on ListNewMembersForNewRoom — refer to ListAddMemberCandidates (the current contract) instead of the removed ListNewMembers. - docs/superpowers: rename stale `_orgRaw` → `orgRaw` in both the plan snippet and the spec narrative to match the renamed BSON tag; add language identifiers (```sh) to the four shell-command fenced blocks the reviewer called out in the plan. Skipped intentionally: - Makefile COMPOSE_BAKE finding: the user explicitly asked to REMOVE COMPOSE_BAKE/COMPOSE_PARALLEL_LIMIT (commit 3df2b19); bot's suggestion to set COMPOSE_BAKE=true contradicts that directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
vjauhari-work
force-pushed
the
claude/fix-member-subscription-bug-QZhjc
branch
from
2f1bad1 to
a4730cc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/client-api.md (1)
196-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate Room docs now reference undocumented request fields (
orgs/users).Line 249 introduces
"invalid org"/"user not found"for channel-create inputs, but the request schema above doesn’t documentorgsorusers. Please update the Create Room request-body table/example to include these fields (or remove that error note if they are not accepted) so the contract is self-consistent.Also applies to: 249-249
🤖 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/client-api.md` around lines 196 - 203, The Create Room docs refer to errors for "invalid org" / "user not found" but the request-body table for the Create Room endpoint omits the `orgs` and `users` fields; either add `orgs` and `users` to the table with their types (e.g., `orgs`: string[] optional, `users`: string[] optional) and notes on validation (when `type=channel`/membership checks) or remove the error text that references them; update the Create Room request-body example and the table entries near the `members`, `type`, and `createdByAccount` fields so the schema and the error notes are consistent with each other.
🧹 Nitpick comments (1)
pkg/testutil/testimages/testimages.go (1)
23-32: ⚡ Quick winPrefer pinning immutable container image tags for integration-test determinism.
pkg/testutil/testimages/testimages.goanddocker-local/compose.deps.yamlboth use floating tags (mongo:8,valkey/valkey:8-alpine). Pin exact patch versions (or image digests) to prevent CI drift across time.🤖 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/testutil/testimages/testimages.go` around lines 23 - 32, The test images are using floating tags causing CI drift; update the Mongo constant and any other floating test image references (e.g., the Mongo variable in pkg/testutil/testimages/testimages.go and the valkey image in docker-local/compose.deps.yaml) to pinned immutable identifiers — preferably exact patch tags or image digests — by replacing "mongo:8" with a specific pinned tag (or digest) for the current CI baseline and doing the same for "valkey/valkey:8-alpine" in compose.deps.yaml; ensure you document the chosen pinned versions in a comment near the Mongo constant and update any CI/README notes that reference the floating tags.
🤖 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.
Outside diff comments:
In `@docs/client-api.md`:
- Around line 196-203: The Create Room docs refer to errors for "invalid org" /
"user not found" but the request-body table for the Create Room endpoint omits
the `orgs` and `users` fields; either add `orgs` and `users` to the table with
their types (e.g., `orgs`: string[] optional, `users`: string[] optional) and
notes on validation (when `type=channel`/membership checks) or remove the error
text that references them; update the Create Room request-body example and the
table entries near the `members`, `type`, and `createdByAccount` fields so the
schema and the error notes are consistent with each other.
---
Nitpick comments:
In `@pkg/testutil/testimages/testimages.go`:
- Around line 23-32: The test images are using floating tags causing CI drift;
update the Mongo constant and any other floating test image references (e.g.,
the Mongo variable in pkg/testutil/testimages/testimages.go and the valkey image
in docker-local/compose.deps.yaml) to pinned immutable identifiers — preferably
exact patch tags or image digests — by replacing "mongo:8" with a specific
pinned tag (or digest) for the current CI baseline and doing the same for
"valkey/valkey:8-alpine" in compose.deps.yaml; ensure you document the chosen
pinned versions in a comment near the Mongo constant and update any CI/README
notes that reference the floating tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d5d4360-7578-41c7-812a-6e02ad97ec29
📒 Files selected for processing (46)
Makefilechat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.jsxchat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.test.jsxdocker-local/cassandra/init/10-table-messages_by_room.cqldocker-local/cassandra/init/11-table-thread_messages_by_room.cqldocker-local/cassandra/init/12-table-pinned_messages_by_room.cqldocker-local/cassandra/init/13-table-messages_by_id.cqldocs/cassandra_message_model.mddocs/client-api.mddocs/superpowers/plans/2026-05-19-member-add-improvements-plan.mddocs/superpowers/specs/2026-05-19-org-to-individual-membership-upgrade-design.mdhistory-service/docker-local/docker-compose.ymlhistory-service/internal/cassrepo/integration_test.gohistory-service/internal/cassrepo/messages_by_id_integration_test.gohistory-service/internal/cassrepo/messages_by_room.gohistory-service/internal/cassrepo/thread_messages.gohistory-service/internal/cassrepo/thread_messages_integration_test.gohistory-service/internal/service/integration_test.goinbox-worker/handler_test.gopkg/displayfmt/combine.gopkg/displayfmt/combine_test.gopkg/model/cassandra/message.gopkg/model/cassandra/message_test.gopkg/model/model_test.gopkg/model/room.gopkg/model/user.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/roomkeymetrics/metrics.gopkg/testutil/testimages/testimages.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.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.goroom-worker/sysmsg.goroom-worker/sysmsg_test.go
💤 Files with no reviewable changes (13)
- pkg/model/cassandra/message.go
- docker-local/cassandra/init/13-table-messages_by_id.cql
- docker-local/cassandra/init/12-table-pinned_messages_by_room.cql
- history-service/docker-local/docker-compose.yml
- pkg/model/cassandra/message_test.go
- history-service/internal/cassrepo/integration_test.go
- pkg/model/room.go
- pkg/roomkeymetrics/metrics.go
- docker-local/cassandra/init/11-table-thread_messages_by_room.cql
- chat-frontend/src/api/types.ts
- docs/cassandra_message_model.md
- docker-local/cassandra/init/10-table-messages_by_room.cql
- chat-frontend/src/api/fetchSidebarBuckets/index.ts
✅ Files skipped from review due to trivial changes (2)
- room-worker/mock_store_test.go
- room-service/mock_store_test.go
vjauhari-work
pushed a commit
that referenced
this pull request
May 21, 2026
Close the consistency gap CodeRabbit flagged on PR #212: the Create Room request schema was missing users/orgs/channels even though the handler accepts them and the same section's error envelope mentions "invalid org" / "user not found" rejections for those exact fields. Add them to the request-body table (marked channel-only and optional, matching the gate the worker enforces) and update the example JSON so the schema, example, and error envelope agree.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/client-api.md`:
- Around line 203-205: Update the create-room documentation so its behavior
matches the new request schema: replace the sentence that currently claims
creation "only enrolls the owner" with a description that creation will enroll
members provided via the users, orgs, and channels fields (listing each field
name and briefly how they are processed and their error responses: users =>
internal user IDs, rejects with "user not found"; orgs => org IDs expanded to
members, rejects with "invalid org" if zero users; channels => ChannelRef copies
members from specified room/site). Ensure the create-room section references the
same enrollment semantics and error messages described in the
users/orgs/channels table so clients implement the correct behavior.
- Around line 200-201: The docs incorrectly mark createdByAccount as required;
update the API table and any create-room documentation to mark
`createdByAccount` as optional (or "no") and add a short note that the
create-room flow derives the creator from the request subject (e.g.,
"create-room uses subject account `chat.user.{account}.request...` so
`createdByAccount` is not required for backend integration requests"); reference
the `createdByAccount` field and the `create-room` flow in the update so clients
know the server uses the subject account and that `siteId` remains required.
🪄 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: 15c7557d-1463-4181-982e-6a71601991ee
📒 Files selected for processing (10)
docs/client-api.mdroom-service/integration_test.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.goroom-worker/integration_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.gotools/loadgen/main_test.go
💤 Files with no reviewable changes (6)
- room-service/store.go
- room-service/mock_store_test.go
- room-worker/store_mongo.go
- room-worker/store.go
- room-worker/mock_store_test.go
- room-service/store_mongo.go
vjauhari-work
added a commit
that referenced
this pull request
May 21, 2026
- pkg/displayfmt/combine_test.go: switch to in-package tests (`package displayfmt`) per CLAUDE.md §4 rule; drop the now-unused local import. - room-worker/handler.go: wrap bare `return nil, reconcileErr` in processSyncCreateDM's dup-key reconcile branch with sync-DM context (CLAUDE.md §3 "Never return bare err"). - room-service/integration_test.go: make two dept-only test cases actually dept-only by setting SectID to a non-matching value (was `dept-x`, so the tests would have passed via sectId match alone — defeating the intent of "dept-only branch is doing its job"). - room-worker/store.go: refresh stale doc on ListNewMembersForNewRoom — refer to ListAddMemberCandidates (the current contract) instead of the removed ListNewMembers. - docs/superpowers: rename stale `_orgRaw` → `orgRaw` in both the plan snippet and the spec narrative to match the renamed BSON tag; add language identifiers (```sh) to the four shell-command fenced blocks the reviewer called out in the plan. Skipped intentionally: - Makefile COMPOSE_BAKE finding: the user explicitly asked to REMOVE COMPOSE_BAKE/COMPOSE_PARALLEL_LIMIT (commit 3df2b19); bot's suggestion to set COMPOSE_BAKE=true contradicts that directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
vjauhari-work
pushed a commit
that referenced
this pull request
May 21, 2026
Close the consistency gap CodeRabbit flagged on PR #212: the Create Room request schema was missing users/orgs/channels even though the handler accepts them and the same section's error envelope mentions "invalid org" / "user not found" rejections for those exact fields. Add them to the request-body table (marked channel-only and optional, matching the gate the worker enforces) and update the example JSON so the schema, example, and error envelope agree.
vjauhari-work
force-pushed
the
claude/fix-member-subscription-bug-QZhjc
branch
from
d531215 to
c1e1e94
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/client-api.md (1)
202-206:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCreate Room behavior text is still internally inconsistent.
Lines 202–206 and Line 255 now document member enrollment during create, but Line 263 still says “creating a room only enrolls the owner.” Please align that sentence with the updated request semantics.
Also applies to: 255-255
🤖 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/client-api.md` around lines 202 - 206, Update the outdated sentence that reads "creating a room only enrolls the owner." to match the new Create Room semantics described earlier (the `users`, `orgs`, and `channels` body fields and the `{account}` subject segment). Replace that sentence with a concise statement that creating a room enrolls the owner (from chat.user.{account} subject) and any additional members specified via the `users`, `orgs`, or `channels` fields (with the same rejection behaviors already documented).
🤖 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-19-org-to-individual-membership-upgrade-design.md`:
- Around line 259-260: The doc contains conflicting statements about the
`_orgMatch` $lookup inner pipeline stage count (one place says 3 stages:
match→addFields→group, another says 6 stages); decide which is correct and make
both references consistent: update the description at the earlier mention (the
paragraph that claims "stays at 3 stages — match → addFields → group") and the
later mention (the paragraph that claims "grows to 6 stages") to reflect the
true pipeline shape used by the `_orgMatch` $lookup, and if the pipeline has
changed, also update the emitted shape note (`{name, tcName, memberCount}`) and
the statement about Go decode using `combineWithFallback` so all three places
describe the same stage count and output shape.
In `@room-worker/handler.go`:
- Around line 935-950: The backfill branch assumes FindUsersByAccounts(ctx,
backfillAccounts) returns every requested account but it can return a subset;
modify the logic in the handler where backfillUsers and backfillAccounts are
used (around the loop that appends to roomMembers) to detect missing accounts
and return an error instead of proceeding: after obtaining backfillUsers, build
a set of returned accounts/IDs, compare its size or membership against
backfillAccounts, and if any requested account is missing return a clear error
(e.g., "missing backfill user(s) for accounts ...") so BulkCreateRoomMembers and
later hadOrgsBefore cannot commit a partial backfill that becomes irrecoverable.
Ensure you reference FindUsersByAccounts, backfillAccounts, backfillUsers,
roomMembers, BulkCreateRoomMembers and fail fast before appending or committing.
---
Duplicate comments:
In `@docs/client-api.md`:
- Around line 202-206: Update the outdated sentence that reads "creating a room
only enrolls the owner." to match the new Create Room semantics described
earlier (the `users`, `orgs`, and `channels` body fields and the `{account}`
subject segment). Replace that sentence with a concise statement that creating a
room enrolls the owner (from chat.user.{account} subject) and any additional
members specified via the `users`, `orgs`, or `channels` fields (with the same
rejection behaviors already documented).
🪄 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: aa202e5c-0c58-468f-bc66-0526f423fd6e
📒 Files selected for processing (47)
Makefilechat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.jsxchat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.test.jsxdocker-local/cassandra/init/10-table-messages_by_room.cqldocker-local/cassandra/init/11-table-thread_messages_by_room.cqldocker-local/cassandra/init/12-table-pinned_messages_by_room.cqldocker-local/cassandra/init/13-table-messages_by_id.cqldocs/cassandra_message_model.mddocs/client-api.mddocs/superpowers/plans/2026-05-19-member-add-improvements-plan.mddocs/superpowers/spec.mddocs/superpowers/specs/2026-05-19-org-to-individual-membership-upgrade-design.mdhistory-service/internal/cassrepo/integration_test.gohistory-service/internal/cassrepo/messages_by_id_integration_test.gohistory-service/internal/cassrepo/messages_by_room.gohistory-service/internal/cassrepo/thread_messages.gohistory-service/internal/cassrepo/thread_messages_integration_test.gohistory-service/internal/service/integration_test.goinbox-worker/handler_test.gopkg/displayfmt/combine.gopkg/displayfmt/combine_test.gopkg/model/cassandra/message.gopkg/model/cassandra/message_test.gopkg/model/member.gopkg/model/model_test.gopkg/model/room.gopkg/model/user.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/roomkeymetrics/metrics.gopkg/testutil/testimages/testimages.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.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.goroom-worker/sysmsg.goroom-worker/sysmsg_test.go
💤 Files with no reviewable changes (14)
- docs/superpowers/spec.md
- pkg/model/cassandra/message.go
- docker-local/cassandra/init/12-table-pinned_messages_by_room.cql
- docs/cassandra_message_model.md
- pkg/model/cassandra/message_test.go
- docker-local/cassandra/init/13-table-messages_by_id.cql
- docker-local/cassandra/init/10-table-messages_by_room.cql
- pkg/model/member.go
- history-service/internal/cassrepo/integration_test.go
- chat-frontend/src/api/fetchSidebarBuckets/index.ts
- pkg/model/room.go
- chat-frontend/src/api/types.ts
- pkg/roomkeymetrics/metrics.go
- docker-local/cassandra/init/11-table-thread_messages_by_room.cql
✅ Files skipped from review due to trivial changes (3)
- Makefile
- history-service/internal/cassrepo/thread_messages.go
- room-worker/mock_store_test.go
vjauhari-work
pushed a commit
that referenced
this pull request
May 21, 2026
room-worker/handler.go (Major):
Backfill in processAddMembers fed FindUsersByAccounts the existing
subscriber accounts and assumed the result covered every requested
one. When a stale subscription points at a deleted user document the
call returns a subset; the buggy code committed room_members for the
rows it got, BulkCreateRoomMembers flipped hadOrgsBefore=true, and
no future redelivery could ever repair the missing IRM rows (backfill
only fires on the first-org transition). Add a fail-hard guard that
compares returned accounts against the requested set and returns
newPermanent("backfill user %s not found ...") before any write. New
regression test in handler_test.go locks in the contract.
docs/client-api.md:
Replace the stale "creating a room only enrolls the owner" line — it
contradicts the request schema introduced earlier this PR, which now
documents users/orgs/channels enrollment on create.
docs/superpowers/specs/2026-05-19-...-upgrade-design.md:
Reconcile contradictory stage-count claims for the _orgMatch $lookup
inner pipeline. The later "grows to 6 stages" sentence was wrong;
verified against room-service/store_mongo.go:520-552 — the pipeline
is 3 stages ($match → $addFields → $group), same as before. The
$group merely widens its accumulator set.
pkg/testutil/testimages/testimages.go + docker-local/compose.deps.yaml:
Replace the floating "mongo:8" / "valkey/valkey:8-alpine" tags with
patch-pinned mongo:8.2.9 / valkey/valkey:8.1.7-alpine so testcontainers
can't drift across patch releases between CI runs.
vjauhari-work
added a commit
that referenced
this pull request
May 21, 2026
- pkg/displayfmt/combine_test.go: switch to in-package tests (`package displayfmt`) per CLAUDE.md §4 rule; drop the now-unused local import. - room-worker/handler.go: wrap bare `return nil, reconcileErr` in processSyncCreateDM's dup-key reconcile branch with sync-DM context (CLAUDE.md §3 "Never return bare err"). - room-service/integration_test.go: make two dept-only test cases actually dept-only by setting SectID to a non-matching value (was `dept-x`, so the tests would have passed via sectId match alone — defeating the intent of "dept-only branch is doing its job"). - room-worker/store.go: refresh stale doc on ListNewMembersForNewRoom — refer to ListAddMemberCandidates (the current contract) instead of the removed ListNewMembers. - docs/superpowers: rename stale `_orgRaw` → `orgRaw` in both the plan snippet and the spec narrative to match the renamed BSON tag; add language identifiers (```sh) to the four shell-command fenced blocks the reviewer called out in the plan. Skipped intentionally: - Makefile COMPOSE_BAKE finding: the user explicitly asked to REMOVE COMPOSE_BAKE/COMPOSE_PARALLEL_LIMIT (commit 3df2b19); bot's suggestion to set COMPOSE_BAKE=true contradicts that directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
vjauhari-work
pushed a commit
that referenced
this pull request
May 21, 2026
Close the consistency gap CodeRabbit flagged on PR #212: the Create Room request schema was missing users/orgs/channels even though the handler accepts them and the same section's error envelope mentions "invalid org" / "user not found" rejections for those exact fields. Add them to the request-body table (marked channel-only and optional, matching the gate the worker enforces) and update the example JSON so the schema, example, and error envelope agree.
vjauhari-work
pushed a commit
that referenced
this pull request
May 21, 2026
room-worker/handler.go (Major):
Backfill in processAddMembers fed FindUsersByAccounts the existing
subscriber accounts and assumed the result covered every requested
one. When a stale subscription points at a deleted user document the
call returns a subset; the buggy code committed room_members for the
rows it got, BulkCreateRoomMembers flipped hadOrgsBefore=true, and
no future redelivery could ever repair the missing IRM rows (backfill
only fires on the first-org transition). Add a fail-hard guard that
compares returned accounts against the requested set and returns
newPermanent("backfill user %s not found ...") before any write. New
regression test in handler_test.go locks in the contract.
docs/client-api.md:
Replace the stale "creating a room only enrolls the owner" line — it
contradicts the request schema introduced earlier this PR, which now
documents users/orgs/channels enrollment on create.
docs/superpowers/specs/2026-05-19-...-upgrade-design.md:
Reconcile contradictory stage-count claims for the _orgMatch $lookup
inner pipeline. The later "grows to 6 stages" sentence was wrong;
verified against room-service/store_mongo.go:520-552 — the pipeline
is 3 stages ($match → $addFields → $group), same as before. The
$group merely widens its accumulator set.
pkg/testutil/testimages/testimages.go + docker-local/compose.deps.yaml:
Replace the floating "mongo:8" / "valkey/valkey:8-alpine" tags with
patch-pinned mongo:8.2.9 / valkey/valkey:8.1.7-alpine so testcontainers
can't drift across patch releases between CI runs.
vjauhari-work
force-pushed
the
claude/fix-member-subscription-bug-QZhjc
branch
from
109bbc5 to
d963eb8
Compare
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/client-api.md`:
- Line 262: Update the Room schema documentation: change the userCount field
note (in the Room schema table) to indicate that userCount defaults to 1 for
owner-only creates but may be greater than 1 when create-room includes initial
members via users, orgs, or channels; reference the create-room behavior and the
userCount field so the table text explicitly states "1 (owner) or higher if
initial members were provided at creation."
🪄 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: 0e9d88b7-d31b-4cf0-beb0-99153f8c047c
📒 Files selected for processing (48)
Makefilechat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.jsxchat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.test.jsxdocker-local/cassandra/init/10-table-messages_by_room.cqldocker-local/cassandra/init/11-table-thread_messages_by_room.cqldocker-local/cassandra/init/12-table-pinned_messages_by_room.cqldocker-local/cassandra/init/13-table-messages_by_id.cqldocker-local/compose.deps.yamldocs/cassandra_message_model.mddocs/client-api.mddocs/superpowers/plans/2026-05-19-member-add-improvements-plan.mddocs/superpowers/spec.mddocs/superpowers/specs/2026-05-19-org-to-individual-membership-upgrade-design.mdhistory-service/internal/cassrepo/integration_test.gohistory-service/internal/cassrepo/messages_by_id_integration_test.gohistory-service/internal/cassrepo/messages_by_room.gohistory-service/internal/cassrepo/thread_messages.gohistory-service/internal/cassrepo/thread_messages_integration_test.gohistory-service/internal/service/integration_test.goinbox-worker/handler_test.gopkg/displayfmt/combine.gopkg/displayfmt/combine_test.gopkg/model/cassandra/message.gopkg/model/cassandra/message_test.gopkg/model/member.gopkg/model/model_test.gopkg/model/room.gopkg/model/user.gopkg/pipelines/member.gopkg/pipelines/member_test.gopkg/roomkeymetrics/metrics.gopkg/testutil/testimages/testimages.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.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.goroom-worker/sysmsg.goroom-worker/sysmsg_test.go
💤 Files with no reviewable changes (14)
- pkg/model/room.go
- docker-local/cassandra/init/10-table-messages_by_room.cql
- chat-frontend/src/api/types.ts
- pkg/model/cassandra/message.go
- docs/superpowers/spec.md
- docker-local/cassandra/init/13-table-messages_by_id.cql
- pkg/model/cassandra/message_test.go
- docker-local/cassandra/init/12-table-pinned_messages_by_room.cql
- history-service/internal/cassrepo/integration_test.go
- docker-local/cassandra/init/11-table-thread_messages_by_room.cql
- pkg/model/member.go
- chat-frontend/src/api/fetchSidebarBuckets/index.ts
- docs/cassandra_message_model.md
- pkg/roomkeymetrics/metrics.go
✅ Files skipped from review due to trivial changes (4)
- pkg/displayfmt/combine.go
- room-worker/mock_store_test.go
- inbox-worker/handler_test.go
- room-service/mock_store_test.go
mliu33
reviewed
May 21, 2026
… cleanups
Five-part spec, perf-hardened per second /simplify pass.
Parts 1-3: org→individual silent no-op fix, deptId support with
prefer-dept-on-overlap, and CreateRoomDialog dedup-reply short-circuit.
New GetCapacityCheckPipeline + GetAddMemberCandidatesPipeline split
capacity-counting (3 stages, no room_members lookup) from full-candidate
resolution (4 stages, room_members lookup via member.id covered by the
existing unique index). New (deptId, account) index keeps both branches
of the extended $or index-covered. Enrichment _orgMatch pipeline stays
at 3 inner stages by emitting raw {name, tcName} and combining in Go;
combineWithFallback moves to pkg/displayfmt as the single source of
truth for both sys-msg and member-list display.
Part 4: drops Room.CreatedBy and rewrites the JetStream replay-equivalence
check to use a reusable reconcileRoomOnDuplicateKey helper. The new check
verifies (Type, SiteID) structural match and requester-sub-exists; this
fixes DM concurrent-create (today incorrectly rejected as "room ID
collision") while still catching real ID collisions (requester has no sub
in the unrelated existing room).
Part 5: drops never-written target_user column from all four Cassandra
message tables, the Go struct, history-service read-side baseColumns,
and the docs.
All new queries verified index-covered; performance/indexes table in
Part 2 enumerates every read path.
14 tasks across five parts mirroring the spec, hardened per the second
/simplify pass.
Notable updates over the first draft:
- pkg/displayfmt shared package introduced in Task 7 — single source of
truth for CombineWithFallback, consumed by both room-worker (sys-msgs)
and room-service (member-list enrichment).
- Task 11 enrichment pipeline kept at 3 inner stages (vs. the earlier
6-stage draft) by emitting raw {name, tcName} and applying the
combine in Go after cursor.All.
- Task 13 (Room.CreatedBy removal) extracts the duplicate-key recovery
into a single Handler.reconcileRoomOnDuplicateKey helper used by both
processCreateRoom and the sync-DM path. Adds a "no extra writes"
pre/post snapshot assertion to guard against silent-dedup false
passes. Frontend and docs/client-api.md grep uses word-boundary regex
to avoid stripping the unrelated createdByAccount field.
- Task 14 (target_user removal) adds explicit test-fixture cleanup
steps covering pkg/model/cassandra/message_test.go and the two
history-service integration tests that today assert on .TargetUser.
TDD per task: failing test → minimal implementation → green → commit.
Each task is independently committable and reviewable.
Adds AddMemberCandidate type, ListAddMemberCandidates to SubscriptionStore
interface and MongoStore implementation backed by GetAddMemberCandidatesPipeline,
returning per-user {hasSubscription, hasIndividualRoomMember} flags so the
handler can split candidates into needSub vs needIRM upgrade paths. Regenerates
mock and adds integration test covering the bug scenario (sub-without-IRM) plus
new-member and full-member states. Also includes room-service CountNewMembers
integration test from the preceding task that was not yet committed.
https://claude.ai/code/session_01Wn4D4EcfZ5nKAuF6vbSBsA
…wait When the server replies with "dm already exists", the handleSubmit now calls onCreated+onClose immediately and returns, bypassing setPendingRoom entirely. This prevents the 3-second "taking longer than expected" banner from firing on a BUCKETS_LOADED race where summaries is still empty at the moment the dedup response lands. https://claude.ai/code/session_01Wn4D4EcfZ5nKAuF6vbSBsA
Both member.add and channel-create silently accepted orgs/users with no backing user document. The candidates aggregation dropped them, so the async-job reply reported success while the worker either wrote a room_members row for a zero-user org (firing a spurious members_added sys-msg) or skipped the targeted user entirely. Gate at the room-service request boundary with two new store methods (FindExistingOrgIDs, FindExistingAccounts) and two handler helpers (validateOrgIDs, validateAccountsExist). errInvalidOrg / errUserNotFound sentinels were already in sanitizeError's allow-list; clients now see a clean synchronous error envelope and nothing reaches the canonical stream. Also folds in two follow-ups from the prior review pass: - Mongo testimage comment now records that the retired 4.4.15 guard against partialFilterExpression operator-allow-list regressions has been intentionally given up under the mongo:8 bump. - ListOrgMembers integration test gains a "neither sectId nor deptId matches" sub-test so dedupe across the broadened \$or is exercised. Spec: docs/superpowers/specs/2026-05-19-org-to-individual-membership-upgrade-design.md Part 6 Plan: docs/superpowers/plans/2026-05-19-member-add-improvements-plan.md Tasks 15 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three unresolved threads from @mliu33 on the merged room-encryption-keys PR. 1. shouldRotate guard (room-worker/handler.go:319): kept the condition; the premise that "JetStream won't redeliver after rotate succeeds" misses three NAK sites in processRemoveIndividual (GetUser, sys-msg publish, cross-site outbox) and the symmetric ones in processRemoveOrg. Without the guard, redelivery re-runs rotateAndFanOut which calls GenerateKeyPair() — fresh random bytes — and survivors briefly offline during the second fan-out are stranded on the prior key. Tightened the inline comment to record the rationale; no behavior change. 2. buildAndFanOutRoomKey redundant Get (room-worker/handler.go:1789): accepted. Refactored to take *VersionedKeyPair as a parameter and threaded the gate-Get pair from processCreateRoom through processCreateRoomChannel + finishCreateRoom. Saves one Valkey round trip per channel/DM create. processAddMembers fetches the pair locally before fan-out so the contract stays uniform (caller owns the fetch). Kept the nil-check inside as a defensive guard. 3. KeyGenerated / KeyRotated success counters: dropped at every emit site (room-worker x3, room-service x1) and removed the declarations from pkg/roomkeymetrics. Error counters (FanoutErrors, ValkeyErrors, KeyAbsentErrors) stay. Spec: docs/superpowers/specs/2026-05-19-org-to-individual-membership-upgrade-design.md Part 7 Plan: docs/superpowers/plans/2026-05-19-member-add-improvements-plan.md Tasks 16-19 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…phantom-validation tests - pkg/pipelines: convert GetCapacityCheckPipeline / GetAddMemberCandidatesPipeline from panic-on-empty-roomID to (bson.A, error); library code should not crash the worker on bad caller input. Extract bot/pseudo regex into package-level const so GetNewMembersPipeline and matchCandidates can't drift. - room-worker/processAddMembers: propagate backfill query errors instead of log-and-continue. Log-and-continue would silently corrupt room_members (existing subs would never get IRM rows). Retry is safe — subs are already written so needSub is empty, hadOrgsBefore stays false until BulkCreateRoomMembers commits, and the backfill re-runs cleanly. - room-worker: requestID propagation on the two member-add publish-error logs now inside the gated block; wrap bare returns after reconcileRoomOnDuplicateKey and rotateAndFanOut; refine error-wrap messages on the dup-key fetch path. Trim BulkCreateSubscriptions doc to one why-line. Project u.account on GetSubscriptionAccounts. - room-service: rename _orgRaw BSON tag to orgRaw, add omitempty across orgRawDisplay siblings. Clarify validateOrgIDs / validateAccountsExist godoc to record the fmt.Errorf wrap-with-input shape that callers see via errors.Is. - room-service tests: consolidate 5 sibling AddMembers phantom tests into a single table-driven case; add parallel CreateRoomChannel phantom table so the same gate is exercised on both paths; add store-error branch cases to cover the FindExistingOrgIDs / FindExistingAccounts error propagation path (was untested per CLAUDE.md §4). - room-worker tests: add backfill-error fail-hard tests for both GetSubscriptionAccounts and FindUsersByAccounts failure paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Removes the inline `COMPOSE_BAKE=false COMPOSE_PARALLEL_LIMIT=1` env-vars from both branches of the `up` target. The Docker Compose CLI applies its own defaults; pinning them here predates the local toolchain and is no longer needed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- pkg/displayfmt/combine_test.go: switch to in-package tests (`package displayfmt`) per CLAUDE.md §4 rule; drop the now-unused local import. - room-worker/handler.go: wrap bare `return nil, reconcileErr` in processSyncCreateDM's dup-key reconcile branch with sync-DM context (CLAUDE.md §3 "Never return bare err"). - room-service/integration_test.go: make two dept-only test cases actually dept-only by setting SectID to a non-matching value (was `dept-x`, so the tests would have passed via sectId match alone — defeating the intent of "dept-only branch is doing its job"). - room-worker/store.go: refresh stale doc on ListNewMembersForNewRoom — refer to ListAddMemberCandidates (the current contract) instead of the removed ListNewMembers. - docs/superpowers: rename stale `_orgRaw` → `orgRaw` in both the plan snippet and the spec narrative to match the renamed BSON tag; add language identifiers (```sh) to the four shell-command fenced blocks the reviewer called out in the plan. Skipped intentionally: - Makefile COMPOSE_BAKE finding: the user explicitly asked to REMOVE COMPOSE_BAKE/COMPOSE_PARALLEL_LIMIT (commit 3df2b19); bot's suggestion to set COMPOSE_BAKE=true contradicts that directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Close the consistency gap CodeRabbit flagged on PR #212: the Create Room request schema was missing users/orgs/channels even though the handler accepts them and the same section's error envelope mentions "invalid org" / "user not found" rejections for those exact fields. Add them to the request-body table (marked channel-only and optional, matching the gate the worker enforces) and update the example JSON so the schema, example, and error envelope agree.
… methods All production write paths now use Bulk* variants (BulkCreateSubscriptions, BulkCreateRoomMembers); the single-row CreateSubscription, CreateRoom (room- service only), and CreateRoomMember linger on the store interfaces with zero live callers and are only kept alive by integration-test seeding. Remove them from the interface + Mongo impl, point the test seed sites at the existing mustInsertRoom / mustInsertSub helpers (direct collection inserts), and drop the now-misleading "existing methods (invite flow)" section comment in room-worker/store.go. Mocks regenerated; lint, unit tests, and integration tests all green locally.
PR #171 follow-up Finding 1: room-service stamped the current Valkey key version into RemoveMemberRequest.BaseKeyVersion and room-worker skipped rotation when Valkey was already past that baseline. The reviewer pointed out that JetStream only redelivers on NAK and a single delivery only ever reaches one pod, so the post-rotate redelivery scenario the guard was shielding against can't actually arise — and the extra Valkey Get on the hot remove path is unnecessary work. Drop the whole machinery: remove BaseKeyVersion from RemoveMemberRequest, delete the keyStore.Get-and-stamp block in room-service/handleRemoveMember, drop the shouldRotate computation and parameter threading through processRemoveOrg / processRemoveIndividual in room-worker, and unconditionally rotate-and-fan-out (the existing `len(accounts) > 0` guard on the org path stays since rotation is genuinely a no-op when nothing was deleted). Spec + plan updated: Finding 1 is removed from Part 7 in both the design doc and the plan, the remaining two findings are renumbered, and the "Skip-rotation guard" test in room-worker plus the "StampsBaseKeyVersion" test in room-service are deleted. The two ProcessRemoveIndividual content tests pick up a ListByRoom expectation because the rotate path now always runs.
Neither field exists on the wire. Room.CreatedBy was removed from the
model earlier in this PR (Task 13), and createdByAccount was never a
field on CreateRoomRequest — the creator's account is taken from the
{account} segment of the create-room subject by ParseRoomCreateSubject
and never read from the request body.
- docs/client-api.md: remove the createdByAccount row from the Create
Room request-body table and from the example JSON; add a one-liner
pointing readers at the subject-derived creator.
- docs/superpowers/spec.md: remove the stale CreatedBy row from the
Room schema table in section 4.1.
- docs/superpowers/plans/2026-05-19-member-add-improvements-plan.md:
rewrite Step 11/12 — the original plan instructed the grep to spare
createdByAccount on the assumption it was a real field; both names
should be stripped.
Historical design/plan docs (2026-03..2026-05-07) that record the
field as it existed at those points in time are left intact.
room-worker/handler.go (Major):
Backfill in processAddMembers fed FindUsersByAccounts the existing
subscriber accounts and assumed the result covered every requested
one. When a stale subscription points at a deleted user document the
call returns a subset; the buggy code committed room_members for the
rows it got, BulkCreateRoomMembers flipped hadOrgsBefore=true, and
no future redelivery could ever repair the missing IRM rows (backfill
only fires on the first-org transition). Add a fail-hard guard that
compares returned accounts against the requested set and returns
newPermanent("backfill user %s not found ...") before any write. New
regression test in handler_test.go locks in the contract.
docs/client-api.md:
Replace the stale "creating a room only enrolls the owner" line — it
contradicts the request schema introduced earlier this PR, which now
documents users/orgs/channels enrollment on create.
docs/superpowers/specs/2026-05-19-...-upgrade-design.md:
Reconcile contradictory stage-count claims for the _orgMatch $lookup
inner pipeline. The later "grows to 6 stages" sentence was wrong;
verified against room-service/store_mongo.go:520-552 — the pipeline
is 3 stages ($match → $addFields → $group), same as before. The
$group merely widens its accumulator set.
pkg/testutil/testimages/testimages.go + docker-local/compose.deps.yaml:
Replace the floating "mongo:8" / "valkey/valkey:8-alpine" tags with
patch-pinned mongo:8.2.9 / valkey/valkey:8.1.7-alpine so testcontainers
can't drift across patch releases between CI runs.
The Room schema table still claimed userCount is "1 immediately after creation (the owner)", but the same section now documents users / orgs / channels enrollment at create time, so the count can land >1. Rewrite the note to match.
Per @mliu33's review on room-service/store_mongo.go:770. The previous implementation iterated sectIDs, deptIDs, then a separate map → slice loop (three passes). Build the result slice directly inside the sectIDs / deptIDs loops, using the map only as a dedupe set, so we make two passes instead of three and skip the map → slice copy. Behavior unchanged; existing integration test covers it.
…trip Per @mliu33's review: the previous implementation issued two Distinct calls (one on sectId, one on deptId) and then merged + deduped in Go. That's two network round-trips to Mongo per room.member.add and channel-create request, which run on every gate check. Rewrite as a single aggregate using $unionWith. The main pipeline does the sectId distinct, $unionWith folds in the deptId distinct, and a terminal $group dedupes the union server-side. Each $match sits at the top of its own pipeline so the (sectId, account) and (deptId, account) compound indexes both still cover their respective branches — explain() against the integration container confirms DISTINCT_SCAN on both indexes (Mongo even applies $groupByDistinctScan), no COLLSCAN. Net: 1 round-trip instead of 2, same index coverage, less Go-side allocation (no more dedup map + intermediate slices). Added a regression test for the overlap case (same orgID matched via sectId on one user + deptId on another) that the existing suite didn't pin down.
Restore the two parallel Distinct calls from before 95f7bd7. The $unionWith version was benchmarked at ~8.5% wall-clock improvement (885µs → 810µs per call against a local Mongo) for one fewer round-trip, but at the cost of ~55% more Go-side allocations per call and a deeper dependency on Mongo's aggregation optimizer. With the gain that small and the call rate at this gate not on the critical hot path, the simpler two-Distinct form is the better trade: less code, less BSON marshaling, version-agnostic from Mongo 4.4 onward, and explicit debuggability (each Distinct's error names which field failed). Verified TestMongoStore_FindExistingOrgIDs_Integration passes on both mongo:8.2.9 (current default) and mongo:4.4.30, including the new overlap-dedup case added in the previous commit. Comment on the function records why we landed back here so a future reader doesn't re-do the round-trip migration without fresh perf numbers.
vjauhari-work
force-pushed
the
claude/fix-member-subscription-bug-QZhjc
branch
from
95f7bd7 to
aa90024
Compare
mliu33
reviewed
May 21, 2026
mliu33
reviewed
May 21, 2026
Per @mliu33's review on pkg/pipelines/member.go (lines 112 and 135). All four $lookup sub-pipelines in this file end with $match + $limit:1, then the outer pipeline only reads $size of the resulting array to check emptiness — it never touches any field of the joined doc. Mongo was still fetching and shipping the full subscription / room_members document into _sub / _irm / existingSub on every match. Append $project {_id: 1} after each $limit so each entry in the join array drops to ~30 bytes instead of the full doc. Same emptiness check ($eq: []) and same $size semantics work unchanged. Affects: - GetNewMembersPipeline (existingSub lookup) - GetCapacityCheckPipeline (_sub lookup) - GetAddMemberCandidatesPipeline (_sub and _irm lookups) No behavior change; existing unit + integration tests cover all three pipelines (TestMongoStore_FindExistingOrgIDs, ListAddMemberCandidates, CountNewMembers) and remain green.
mliu33
reviewed
May 21, 2026
…NewMembersPipeline Per @mliu33's review on room-service/store_mongo.go:300. The two functions produced byte-identical pipelines for roomID != "" (modulo cosmetic variable names: existingSub/userAccount vs _sub/acct). GetNewMembersPipeline already handles both cases natively — it returns just the $match stage when roomID is empty, and adds the $lookup + not-already-subscribed $match when roomID is non-empty. - pkg/pipelines/member.go: delete GetCapacityCheckPipeline. Refactor GetNewMembersPipeline to use the shared matchCandidates() helper for the $match stage (DRY with GetAddMemberCandidatesPipeline). - pkg/pipelines/member_test.go: delete TestGetCapacityCheckPipeline. Behavior is fully covered by the existing TestGetNewMembersPipeline* cases (both branches). - room-service/store_mongo.go: CountNewMembers no longer needs the roomID-empty bifurcation — it now calls GetNewMembersPipeline unconditionally with the supplied roomID. - docs/superpowers/specs/2026-05-19-...-upgrade-design.md: spec rewritten to reflect the consolidation. The plan doc (a historical record of Task 1 as originally executed) is left intact.
mliu33
reviewed
May 21, 2026
…veOrg Per @mliu33's review on room-worker/handler.go:562. The previous two-pass logic scanned `members` once for a dept row and then (only if none was found) scanned it again for the first sect row. OrgMemberStatus already carries the `IsDept` flag, so we can do this in a single pass: stash the first sect candidate as we scan, then let any encountered dept row override it (and short-circuit further iteration). After the loop, fall back to the stashed sect candidate only if no dept row appeared. No behavior change — every TestHandler_ProcessRemoveOrg_DeptFirstTiebreak subcase (all sect, all dept, mixed dept-wins, all-names-empty → orgID fallback) still passes. Saves one full pass over the members slice on the org-remove hot path.
…g the same user
Multi-org overlap bug introduced by Part 2 of this PR (dept-aware org
matching). Before: orgs matched only on sectId, so a user could be
reachable in a room via at most one org. After Part 2: orgs match
sectId OR deptId, so a user with both fields set can be covered by two
org rows simultaneously (one matching their sectId, one matching their
deptId).
processRemoveOrg's old toRemove rule only checked HasIndividualMembership:
any org-only user was scheduled for sub deletion. With multi-org overlap,
this silently orphans the user's sub even though the sibling org row
still claims them as a member of the room. They lose notifications,
can't see new messages, and effectively get booted — visible only on
the next list-members call where the sibling org still expands to them.
Fix is two-layer:
- room-worker/store.go: add HasOtherOrgMembership bool to OrgMemberStatus.
- room-worker/store_mongo.go: GetOrgMembersWithIndividualStatus gains a
second $lookup against room_members. For each candidate, search for
another org row whose member.id matches their sectId or deptId
(excluding the orgID being removed). Same $project: {_id: 1} trick
as the other lookups to keep the join entry small.
- room-worker/handler.go: processRemoveOrg skips members where
HasOtherOrgMembership=true from toRemove. Org row deletion + count
reconcile + sys-msg still fire (the org WAS removed); no rotate, no
per-account events when toRemove is empty.
Tests:
- Handler unit test (TestHandler_ProcessRemoveOrg_OtherOrgCovers_PreservesSub):
mock returns alice with HasOtherOrgMembership=true; asserts
DeleteSubscriptionsByAccounts and ListByRoom are never called.
- Pipeline integration test
(TestMongoStore_GetOrgMembersWithIndividualStatus_OtherOrgCovers_Integration):
alice has sectId=X + deptId=Y, both X and Y as org rows; queries org
X and asserts alice.HasOtherOrgMembership=true (Y still covers her)
while carol — sectId=X only — has HasOtherOrgMembership=false.
Both tests were red before the corresponding fix and green after; existing
TestHandler_ProcessRemoveOrg* and TestMongoStore_GetOrgMembersWithIndividualStatus*
cases still pass.
mliu33
reviewed
May 21, 2026
Collaborator
mliu33
left a comment
mliu33
left a comment
There was a problem hiding this comment.
Outstanding work, thanks! Just some minor comments
mliu33
reviewed
May 21, 2026
| // delete their subscription. | ||
| {{Key: "$lookup", Value: bson.M{ | ||
| "from": "room_members", | ||
| "let": bson.M{"sectId": "$sectId", "deptId": "$deptId"}, |
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
4 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
handleAddMemberssilently dropped phantom orgs/users viaCountNewMembers's$orfilter; replaces with explicitvalidateOrgIDs/validateAccountsExistgates that return sentinel errors before publishing to the canonical stream. Same gates wired intohandleCreateRoomChannel.sectIdanddeptId(with dept-first tiebreak in display formatting).pkg/pipelines.GetNewMembersPipelineupdated; new(deptId, account)index mirrors the existing(sectId, account).Room.CreatedByand the unused Cassandratarget_usercolumn.reconcileRoomOnDuplicateKeyextracted (drops requester-must-be-member check so mid-write crashes recover via JetStream replay); pair-passing refactor inbuildAndFanOutRoomKey;shouldRotatecomment tightened.pkg/pipelinespanic → error on emptyroomID; backfill query errors now fail-hard instead ofslog.Warn-and-continue (silent corruption risk inroom_members); requestID propagated on the relocated member-add publish-error logs;_orgRawBSON tag renamed toorgRawwithomitemptysiblings.Test plan
make test— all unit tests pass with-racemake lint— 0 issuesmake test-integration— verify against real Mongo/Cassandra/NATSmake sast— note:govulncheckcurrently fails on pre-existinghistory-servicemock drift (internal/modelsimport); not introduced by this branchchat.user.{account}.room.{roomID}.{siteID}.msg.add-memberswith a phantom org — should reject synchronously witherrInvalidOrgNotes
docs/client-api.mdupdated in the same diff: removescreatedByfromPOST /roomsand Room shape examples; adds error envelopes for Add Members and Create Room; documents dept-aware org-resolution forchat.user.{account}.request.orgs.{orgID}.members.CombineWithFallbackrename (semantic naming lives in call-site wrappers); folding validators intoCountNewMembers(defeats the bug-fix's point);MemberRemoved.SectName→OrgNamerename (tracking item, wire-stable);ListByRoomprojection (cascades through interface + mocks for alowfinding).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation