From 15f550a1841646c36babd324445fadb12206c620 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 27 Sep 2026 00:19:52 -0700 Subject: [PATCH 1/2] refactor(membership): one callable spelling, no dead member shapes, and arms built from the real document (TASK-170) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three traps that produced the TASK-165/166 membership defects, closed. (1) `utils/isPodMember` exported the creator-inclusive rule three ways — the default, the named `isPodMember`, and the strict rule beside them — so the obvious spelling `require('./utils/isPodMember')` bound the permissive one; TASK-165's M2 mutation is that edit, and it stayed green. Deleted rather than renamed: the creator clause answered "did `Pod.members` forget this creator", which the pre-save hook already covers, and a live-looking function with no caller is one import away from being reached for again. The module now exports `{ isListedPodMember }` and nothing callable by default, asserted in a new unit suite whose first arm is a positive control on the export object. (2) `getRecap`, `getUserFeed` and `getDecisionHistory` each carried a second spelling of membership (`{ 'members.userId': … }`, `{ 'members._id': … }`) that matched nothing — no pod has either shape (Vera: 0 of 424) — while teaching that a member is an object with a key. That belief is what produced the `getPodFeed` defect TASK-166 fixed, so the terms go, along with the same arm in `getDecisionHistory`'s post-filter: with the query narrowed, an object-shaped member cannot reach that filter, and a reader that admits what the selector does not select is the worse of the two. (3) TASK-166's two `leavePod` arms were built on `members: ['creator','member']`, so `[...pod.members].includes(req.userId)` — a reasonable-looking tightening — would keep both green while refusing every real member: spreading unwraps mongoose's array wrapper into plain ObjectIds, where `pod.members.includes(hex)` casts. The arms now build the pod from the real model, with `isMongooseArray` as the shape control and a fixture-control arm asserting the difference they depend on. M6 in the ledger is that spread edit, and it now reddens the control arm. One test fixture was load-bearing on the dead shape and is disclosed rather than quietly repaired: `attentionQueue`'s durable-read arm mocked a pod whose member was `{ userId: 'member-1' }` — a pod that cannot exist — and narrowing the post-filter turned it red with "Access denied". That is the belief living in the corpus, which is the cleanest evidence for (2)'s premise. --- .../unit/controllers/podController.test.js | 54 +++++++++++------ .../activityService.attentionQueue.test.js | 37 ++++++++---- .../services/activityService.recap.test.js | 10 ++-- ...activityService.userFeedMembership.test.js | 10 ++-- .../__tests__/unit/utils/isPodMember.test.js | 58 +++++++++++++++++++ backend/services/activityService.ts | 38 ++++++------ backend/utils/isPodMember.ts | 38 ++++++------ 7 files changed, 164 insertions(+), 81 deletions(-) create mode 100644 backend/__tests__/unit/utils/isPodMember.test.js diff --git a/backend/__tests__/unit/controllers/podController.test.js b/backend/__tests__/unit/controllers/podController.test.js index 08b7a9873..244725394 100644 --- a/backend/__tests__/unit/controllers/podController.test.js +++ b/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); }); diff --git a/backend/__tests__/unit/services/activityService.attentionQueue.test.js b/backend/__tests__/unit/services/activityService.attentionQueue.test.js index 1c71b74a5..78b21bec8 100644 --- a/backend/__tests__/unit/services/activityService.attentionQueue.test.js +++ b/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 () => { diff --git a/backend/__tests__/unit/services/activityService.recap.test.js b/backend/__tests__/unit/services/activityService.recap.test.js index 1289377c3..e396443cf 100644 --- a/backend/__tests__/unit/services/activityService.recap.test.js +++ b/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 () => { diff --git a/backend/__tests__/unit/services/activityService.userFeedMembership.test.js b/backend/__tests__/unit/services/activityService.userFeedMembership.test.js index c3008d00c..bb5f3133e 100644 --- a/backend/__tests__/unit/services/activityService.userFeedMembership.test.js +++ b/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 () => { diff --git a/backend/__tests__/unit/utils/isPodMember.test.js b/backend/__tests__/unit/utils/isPodMember.test.js new file mode 100644 index 000000000..97055981d --- /dev/null +++ b/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); + }); +}); diff --git a/backend/services/activityService.ts b/backend/services/activityService.ts index 4e774eb6f..90b09e991 100644 --- a/backend/services/activityService.ts +++ b/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)); diff --git a/backend/utils/isPodMember.ts b/backend/utils/isPodMember.ts index 4ba4beab7..cc230af50 100644 --- a/backend/utils/isPodMember.ts +++ b/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 {}; From 6efd9ac98dc1614ec6a5a2445645815ac57e289b Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 27 Sep 2026 00:27:55 -0700 Subject: [PATCH 2/2] docs(membership): the relay policy's comment no longer claims a second export beside it (TASK-170) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by Wren (74715). `services/connectorRelayPolicy.ts` said the permissive `isPodMember` "is deliberately NOT imported any more" and that the strict rule is "Defined beside `isPodMember` in utils" — true when it was written, false as of this PR, where nothing sits beside it. Comment-only, so the six witnesses Vera ran at `15f550a1` stand unchanged; the diff from that head is this one file. Swept for the same stale claim rather than fixing the one site: the remaining references to `isPodMember` in `backend/` are either historical ("Before TASK-161 this read the permissive `isPodMember`"), a different local helper (`pgMessageController`'s `isPodMemberInMongo`), or the AX audit entry that records this exact name collision as history — all accurate. The two plan docs under `docs/plans/` name `isPodMember` as the predicate a gate *should* check; they are dated design records, and the strict rule is what such a gate uses, so they are left alone (flagged to Wren rather than changed unilaterally). --- backend/services/connectorRelayPolicy.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/backend/services/connectorRelayPolicy.ts b/backend/services/connectorRelayPolicy.ts index 598d3533e..9fc780dfb 100644 --- a/backend/services/connectorRelayPolicy.ts +++ b/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');