refactor(membership): one callable spelling, no dead member shapes, and arms built from the real document (TASK-170)

backend/__tests__/unit/controllers/podController.test.js

@@ -283,17 +283,41 @@ describe('podController', () => {
   });
 
   // ── TASK-166: the creator cannot leave, everyone else still can ─────────
+  // TASK-170: these two arms build the pod from the REAL model rather than
+  // `members: ['creator', 'member']`. `leavePod` decides with
+  // `pod.members.includes(req.userId)`, and on a hydrated document mongoose's
+  // array wrapper casts the hex string so the comparison holds. Rewrite it as
+  // `[...pod.members].includes(req.userId)` — a reasonable-looking tightening —
+  // and it refuses every member, because spreading unwraps the wrapper into
+  // plain ObjectIds; with string members both spellings pass, so the fixtures
+  // this replaces could not see that edit. The arm below asserts the shape the
+  // fixture must have, instead of assuming it.
+  const CREATOR_ID = new mongoose.Types.ObjectId();
+  const MEMBER_ID = new mongoose.Types.ObjectId();
+  const hydratedPod = (members) => {
+    const RealPod = jest.requireActual('../../../models/Pod');
+    const doc = new RealPod({ _id: new mongoose.Types.ObjectId(), name: 'Pod', type: 'chat', createdBy: CREATOR_ID, members });
+    // Only the two methods the route reaches for are stubbed; everything the
+    // membership decision touches is the real document.
+    doc.save = jest.fn().mockResolvedValue(doc);
+    doc.populate = jest.fn().mockResolvedValue(doc);
+    return doc;
+  };
+
+  it('the hydrated fixture separates the wrapper predicate from a spread of it', () => {
+    // Fixture control, not production behaviour: it shows the difference the two
+    // arms below depend on. If this passes with a string-array fixture the arms
+    // are blind to the spread edit.
+    const pod = hydratedPod([CREATOR_ID, MEMBER_ID]);
+    expect(pod.members.isMongooseArray).toBe(true);
+    expect(pod.members.includes(String(MEMBER_ID))).toBe(true);
+    expect([...pod.members].includes(String(MEMBER_ID))).toBe(false);
+  });
 
   it('leavePod refuses the creator with 409 creator_cannot_leave and keeps them listed', async () => {
-    const pod = {
-      _id: 'p1',
-      createdBy: 'creator',
-      members: ['creator', 'member'],
-      save: jest.fn(),
-      populate: jest.fn(),
-    };
+    const pod = hydratedPod([CREATOR_ID, MEMBER_ID]);
     Pod.findById.mockResolvedValue(pod);
-    const req = { params: { id: 'p1' }, userId: 'creator' };
+    const req = { params: { id: String(pod._id) }, userId: String(CREATOR_ID) };
     const res = { status: jest.fn().mockReturnThis(), json: jest.fn() };
 
     await podController.leavePod(req, res);
@@ -302,27 +326,21 @@ describe('podController', () => {
     expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ code: 'creator_cannot_leave' }));
     // The refusal is a non-event, not a silent success: nothing was unlisted
     // and nothing was saved.
-    expect(pod.members).toEqual(['creator', 'member']);
+    expect(pod.members.map(String)).toEqual([String(CREATOR_ID), String(MEMBER_ID)]);
     expect(pod.save).not.toHaveBeenCalled();
   });
 
   // Positive control. The arm above is satisfied by a route that refuses
   // everyone, which is exactly the shape the guard must not be.
   it('leavePod still removes a non-creator member (control)', async () => {
-    const pod = {
-      _id: 'p1',
-      createdBy: 'creator',
-      members: ['creator', 'member'],
-      save: jest.fn(),
-      populate: jest.fn().mockResolvedValue(),
-    };
+    const pod = hydratedPod([CREATOR_ID, MEMBER_ID]);
     Pod.findById.mockResolvedValue(pod);
-    const req = { params: { id: 'p1' }, userId: 'member' };
+    const req = { params: { id: String(pod._id) }, userId: String(MEMBER_ID) };
     const res = { status: jest.fn().mockReturnThis(), json: jest.fn() };
 
     await podController.leavePod(req, res);
 
-    expect(pod.members).toEqual(['creator']);
+    expect(pod.members.map(String)).toEqual([String(CREATOR_ID)]);
     expect(pod.save).toHaveBeenCalled();
     expect(res.json).toHaveBeenCalledWith(pod);
   });

backend/__tests__/unit/services/activityService.attentionQueue.test.js

@@ -61,7 +61,7 @@ describe('ActivityService.getDecisionQueue', () => {
 
   it('reads settled decisions durably for a current pod member', async () => {
     mockPodFind.mockReturnValue(chain([{
-      _id: 'pod-1', name: 'Current', createdBy: 'owner', members: [{ userId: 'member-1' }],
+      _id: 'pod-1', name: 'Current', createdBy: 'owner', members: ['member-1'],
     }]));
     mockDecisionCountDocuments.mockResolvedValue(1);
     mockDecisionFind.mockReturnValue(decisionChain([{
@@ -79,7 +79,7 @@ describe('ActivityService.getDecisionQueue', () => {
       ruling: expect.objectContaining({ value: 'B', by: 'Sam', messageId: '43' }),
     })]);
     expect(mockPodFind).toHaveBeenCalledWith(expect.objectContaining({
-      _id: 'pod-1', $or: expect.any(Array),
+      _id: 'pod-1', members: 'member-1',
     }));
     expect(mockDecisionFind).toHaveBeenCalledWith({
       podId: { $in: ['pod-1'] }, status: 'ruled', messageId: { $exists: true },
@@ -99,14 +99,31 @@ describe('ActivityService.getDecisionQueue', () => {
 
     await ActivityService.getDecisionHistory('member-1', { podId: 'pod-1' });
 
-    expect(mockPodFind).toHaveBeenCalledWith({
-      _id: 'pod-1',
-      $or: [
-        { 'members.userId': 'member-1' },
-        { members: 'member-1' },
-        { 'members._id': 'member-1' },
-      ],
-    });
+    // TASK-170: the two dead spellings are gone; exact equality is the falsifier.
+    expect(mockPodFind).toHaveBeenCalledWith({ _id: 'pod-1', members: 'member-1' });
+  });
+
+  it('does not admit a pod whose member is the legacy `{ userId }` shape', async () => {
+    // The post-filter used to carry the same arm as the query term. Nothing in
+    // the store has that shape (0 of 424 pods), and the selector no longer
+    // returns it, so the filter must not be the one place that still admits it.
+    mockPodFind.mockReturnValue(chain([{
+      _id: 'pod-1', name: 'Legacy', createdBy: 'creator-1', members: [{ userId: 'member-1' }],
+    }]));
+
+    await expect(ActivityService.getDecisionHistory('member-1', { podId: 'pod-1' }))
+      .rejects.toThrow('Access denied');
+    expect(mockDecisionFind).not.toHaveBeenCalled();
+  });
+
+  it('admits a pod whose member carries an `_id` (control for the arm above)', async () => {
+    mockPodFind.mockReturnValue(chain([{
+      _id: 'pod-1', name: 'Object id', createdBy: 'creator-1', members: [{ _id: 'member-1' }],
+    }]));
+
+    await ActivityService.getDecisionHistory('member-1', { podId: 'pod-1' });
+
+    expect(mockDecisionFind).toHaveBeenCalled();
   });
 
   it('refuses a creator who left the pod, although createdBy still names them', async () => {

backend/__tests__/unit/services/activityService.recap.test.js

@@ -58,12 +58,10 @@ describe('ActivityService recap and legacy approval authorization', () => {
     // returns whichever fixture it is handed. `createdBy` is written once at
     // creation and survives `leavePod`, so reading it here handed a departed
     // creator the recap of a pod they are no longer in.
-    expect(Pod.find.mock.calls[0][0]).toEqual({
-      $or: [
-        { 'members.userId': ownerId },
-        { members: ownerId },
-      ],
-    });
+    // TASK-170: exact equality, so the arm fails on an added term as well as a
+    // missing one — the dead `{ 'members.userId': … }` spelling cannot come
+    // back without reddening this line.
+    expect(Pod.find.mock.calls[0][0]).toEqual({ members: ownerId });
   });
 
   test('rejects a requested pod that is outside the viewer membership', async () => {

backend/__tests__/unit/services/activityService.userFeedMembership.test.js

@@ -40,12 +40,10 @@ describe('ActivityService.getUserFeed — pod membership', () => {
   it('selects the viewer\'s pods by membership, not by createdBy', async () => {
     await ActivityService.getUserFeed('caller-1', {});
 
-    expect(Pod.find).toHaveBeenCalledWith({
-      $or: [
-        { 'members.userId': 'caller-1' },
-        { members: 'caller-1' },
-      ],
-    });
+    // TASK-170: exact shape — `toHaveBeenCalledWith` on the whole object fails
+    // for an extra term as well as a missing one, so the dead spelling is kept
+    // out by this line rather than by a comment.
+    expect(Pod.find).toHaveBeenCalledWith({ members: 'caller-1' });
   });
 
   it('still reaches the aggregator for a member (control)', async () => {

backend/__tests__/unit/utils/isPodMember.test.js

@@ -0,0 +1,58 @@
+/**
+ * TASK-170 (1).
+ *
+ * This module used to export a second, creator-inclusive rule three ways — the
+ * default, the named `isPodMember`, and `isListedPodMember` beside them — so the
+ * obvious spelling `require('../../utils/isPodMember')` bound the *permissive*
+ * one. TASK-165's M2 mutation is exactly that edit and it stayed green.
+ *
+ * After TASK-166 nothing in production called the permissive rule (8 binders,
+ * all destructuring `isListedPodMember`), so it is deleted. An absence cannot be
+ * shown by execution, so the instrument here is the module's own shape: the
+ * first arm reads the export object, with a positive control so a broken
+ * `require` cannot pass for a deletion.
+ */
+const mod = require('../../../utils/isPodMember');
+const { Types } = require('mongoose');
+
+describe('utils/isPodMember', () => {
+  it('exports no callable default: the bare require cannot reach a permissive rule', () => {
+    expect(typeof mod).toBe('object');
+    expect(typeof mod.isPodMember).toBe('undefined');
+    expect(typeof mod.default).toBe('undefined');
+    // Positive control. The arms above assert an absence, so a `require` that
+    // returned something unexpected would satisfy them for the wrong reason.
+    expect(typeof mod.isListedPodMember).toBe('function');
+  });
+
+  it('refuses a creator who is not listed', () => {
+    const pod = { createdBy: 'creator-1', members: ['member-1'] };
+    expect(mod.isListedPodMember(pod, 'creator-1')).toBe(false);
+  });
+
+  it('admits a creator who is listed: the arm above is not a blanket refusal', () => {
+    const pod = { createdBy: 'creator-1', members: ['creator-1', 'member-1'] };
+    expect(mod.isListedPodMember(pod, 'creator-1')).toBe(true);
+  });
+
+  it('admits a member as an ObjectId and as the same id in hex', () => {
+    // The rule compares on `toString()`, so it does not depend on the mongoose
+    // array wrapper having cast the caller for it. A predicate that only works
+    // through the wrapper is the TASK-170 (3) trap, one layer down.
+    const id = new Types.ObjectId();
+    expect(mod.isListedPodMember({ members: [id] }, String(id))).toBe(true);
+    expect(mod.isListedPodMember({ members: [id] }, id)).toBe(true);
+  });
+
+  it('admits a member document carrying `_id` (the hydrated shape)', () => {
+    const id = new Types.ObjectId();
+    const pod = { members: [{ _id: id }] };
+    expect(mod.isListedPodMember(pod, String(id))).toBe(true);
+  });
+
+  it('refuses a missing pod, a missing caller, and a non-member', () => {
+    expect(mod.isListedPodMember(null, 'user-1')).toBe(false);
+    expect(mod.isListedPodMember({ members: ['user-1'] }, null)).toBe(false);
+    expect(mod.isListedPodMember({ members: ['user-1'] }, 'user-2')).toBe(false);
+  });
+});

backend/services/activityService.ts

@@ -128,12 +128,12 @@ class ActivityService {
     // TASK-166: the pod list is built from membership only. `createdBy` says who
     // made the pod, and `leavePod` keeps it after unlisting them, so reading it
     // as membership handed a departed creator this pod's recap.
-    const pods: PodDoc[] = await Pod.find({
-      $or: [
-        { 'members.userId': userId },
-        { members: userId },
-      ],
-    }).select('_id name type').lean();
+    // TASK-170: `{ 'members.userId': userId }` sat beside the live term as a
+    // second spelling of membership. No pod has a `members.userId` entry (Vera,
+    // 0 of 424, 2026-09-27), so it selected nothing while teaching that a member
+    // is an object carrying a `userId` — the belief behind the `getPodFeed`
+    // defect TASK-166 fixed. Deleted rather than kept as a fallback.
+    const pods: PodDoc[] = await Pod.find({ members: userId }).select('_id name type').lean();
 
     const requestedPodId = typeof options.podId === 'string' ? options.podId : '';
     const scopedPods = requestedPodId
@@ -315,12 +315,9 @@ class ActivityService {
         return { activities: [], hasMore: false, quick: null };
       }
 
-      const pods: PodDoc[] = await Pod.find({
-        $or: [
-          { 'members.userId': userId },
-          { members: userId },
-        ],
-      })
+      // TASK-170: the dead `members.userId` term is gone here too; see
+      // `getRecap` for the census and the reason.
+      const pods: PodDoc[] = await Pod.find({ members: userId })
         .select('_id name type')
         .lean();
 
@@ -410,18 +407,19 @@ class ActivityService {
       : [];
     const limit = Number.isInteger(options.limit) ? Math.min(Math.max(options.limit as number, 1), 50) : 50;
     const offset = Number.isInteger(options.offset) ? Math.max(options.offset as number, 0) : 0;
-    const membership = {
-      $or: [
-        { 'members.userId': userId },
-        { members: userId },
-        { 'members._id': userId },
-      ],
-    };
+    // TASK-170: `members._id` and `members.userId` were the other two spellings
+    // of the same belief (a member is an object with a key) and matched nothing —
+    // no pod carries either shape. The live term is the one below it.
+    const membership = { members: userId };
     const podQuery = requestedPodId ? { _id: requestedPodId, ...membership } : membership;
     const pods: PodDoc[] = await Pod.find(podQuery).select('_id name members').lean();
     if (requestedPodId && pods.length === 0) throw new Error('Access denied');
+    // TASK-170: the post-filter's `member?.userId` arm is the same dead shape as
+    // the query terms above. With the query narrowed, an object-shaped member
+    // can no longer reach this line, so leaving the arm would be a reader that
+    // admits what the selector does not select.
     const allowedPods = pods.filter((pod) => (
-      (pod.members || []).some((member: any) => String(member?.userId || member?._id || member) === String(userId))
+      (pod.members || []).some((member: any) => String(member?._id || member) === String(userId))
     ));
     if (requestedPodId && allowedPods.length === 0) throw new Error('Access denied');
     const podIds = allowedPods.map((pod) => String(pod._id));

backend/services/connectorRelayPolicy.ts

@@ -4,10 +4,13 @@
 // or about which pods a connector may address at all.
 
 // The strict membership rule, re-exported here so every connector site reads one
-// definition through this module. The permissive `isPodMember` is deliberately
-// NOT imported any more: its creator clause is what let a departed creator keep
-// relaying (TASK-161). Defined beside `isPodMember` in utils so the platform
-// readers that need the same rule (PG chat, reactions) share this one home.
+// definition through this module. It is also the only rule its home module exports
+// any more: as of TASK-170 `utils/isPodMember` carries no creator-inclusive
+// export, because that permissive predicate — membership *or* `createdBy` — is
+// what let a departed creator keep relaying (TASK-161). The creator clause it
+// carried is covered where the pod is created (`Pod`'s pre-save hook lists
+// `createdBy`), not by a second predicate, so nothing sits beside
+// `isListedPodMember` now.
 // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require
 const { isListedPodMember } = require('../utils/isPodMember');
 

backend/utils/isPodMember.ts

@@ -3,26 +3,24 @@
 // exists for read observability and would make "only members can write here"
 // untrue for the one account most able to do damage by accident.
 //
-// The creator counts as a member — `Pod.members` does not always list them.
-const isPodMember = (pod: any, userId: unknown): boolean => {
-  if (!pod || !userId) return false;
-  const id = String(userId);
-  if (pod.createdBy?.toString?.() === id) return true;
-  return (pod.members || []).some((m: any) => (
-    (m?._id?.toString?.() || m?.toString?.() || '') === id
-  ));
-};
-
-// The membership rule the pod's own write paths implement, and nothing else:
-// `createMessage` (controllers/messageController.ts) and the socket post path
-// (server.ts) both check `pod.members` alone. A connector that admits a writer
-// those two refuse is more permissive than the pod it writes into, which is what
-// TASK-161 measured: `createdBy` keeps the creator's id after `leavePod` filters
+// The rule the pod's own write paths implement, and nothing else: `createMessage`
+// (controllers/messageController.ts) and the socket post path (server.ts) both
+// check `pod.members` alone. A connector that admits a writer those two refuse
+// is more permissive than the pod it writes into, which is what TASK-161
+// measured: `createdBy` keeps the creator's id after `leavePod` filters
 // `members`, so a departed creator was refused by the app and relayed anyway.
 //
-// Named for the mechanism, not for emphasis. The creator clause above answers
-// "is this someone `Pod.members` forgot to list" — a false negative at creation
-// time — and was never a ruling that leaving leaves membership intact.
+// TASK-170: this module no longer exports a second, creator-inclusive rule.
+// It used to export one twice (`module.exports = isPodMember` and a named
+// `isPodMember`), so the obvious spelling `require('./utils/isPodMember')` bound
+// the permissive one — TASK-165's M2 mutation is exactly that edit, and it stayed
+// green. After TASK-166 nothing in production called it (8 binders, all
+// destructuring `isListedPodMember`), so it is deleted rather than renamed: the
+// clause answered "is this someone `Pod.members` forgot to list", a creation-time
+// false negative that `Pod`'s pre-save hook already covers, and a live-looking
+// function with no caller is one import away from being reached for again.
+// See `__tests__/unit/utils/isPodMember.test.js` — the module has no callable
+// default, and that is asserted rather than assumed.
 const isListedPodMember = (pod: any, userId: unknown): boolean => {
   if (!pod || !userId) return false;
   const id = String(userId);
@@ -31,8 +29,6 @@ const isListedPodMember = (pod: any, userId: unknown): boolean => {
   ));
 };
 
-module.exports = isPodMember;
-module.exports.isPodMember = isPodMember;
-module.exports.isListedPodMember = isListedPodMember;
+module.exports = { isListedPodMember };
 
 export {};