fix(server): tidy collections quirks after the DI fold

server/tests/unit/mcp/tools-collections.test.ts

@@ -5,8 +5,9 @@
  * suite — so this is a characterization of the ported behavior: payload
  * shapes, error texts (the service's thrown httpError messages surfaced via
  * fail()), the owner-only available-users gate, demo denial on writes, the
- * collections read/write scope gating, and the deliberate ABSENCE of an
- * addon gate (the REST surface is addon-gated; the MCP tools never were).
+ * collections read/write scope gating, and — since the post-fold quirk pass —
+ * the collections-addon `when:` gate (the legacy registrar registered
+ * unconditionally while REST and the plugin host gated; the port fixed that).
  */
 import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest';
 
@@ -66,6 +67,9 @@ function clearCollections() {
 beforeAll(async () => {
   createTables(testDb);
   runMigrations(testDb);
+  // The collections addon is seeded DISABLED by default and every tool rides
+  // the `when:` addon gate (post-fold quirk fix) — enable it for the suite.
+  testDb.prepare('UPDATE addons SET enabled = 1 WHERE id = ?').run(ADDON_IDS.COLLECTIONS);
   // Warm the notificationService module: sendInvite does a fire-and-forget
   // dynamic import of it, and a cold load can otherwise race the mock registry
   // under slow (coverage) runs — same precaution as tests/integration/vacay.test.ts.
@@ -509,20 +513,20 @@ describe('Collection tools — scope gating', () => {
 });
 
 // ---------------------------------------------------------------------------
-// Addon NON-gating — parity quirk carried over from the legacy registrar:
-// unlike the REST CollectionsAddonGuard and the plugin host's requireAddon,
-// the MCP tools register even with the collections addon disabled.
+// Addon gating — added with the post-fold quirk pass: the legacy registrar
+// registered unconditionally (unlike the REST CollectionsAddonGuard and the
+// plugin host's requireAddon); every entry now rides the collections addon
+// via the `when:` predicate.
 // ---------------------------------------------------------------------------
 
-describe('Collection tools — no addon gate (legacy parity)', () => {
-  it('still registers the tools when the collections addon is disabled', async () => {
+describe('Collection tools — collections addon gating', () => {
+  it('registers nothing when the collections addon is disabled', async () => {
     const { user } = createUser(testDb);
     testDb.prepare('UPDATE addons SET enabled = 0 WHERE id = ?').run(ADDON_IDS.COLLECTIONS);
     try {
       await withHarness(user.id, async (h) => {
         const names = (await h.client.listTools()).tools.map((t) => t.name);
-        expect(names).toContain('list_collections');
-        expect(names).toContain('create_collection');
+        for (const tool of [...READ_TOOLS, ...WRITE_TOOLS]) expect(names).not.toContain(tool);
       });
     } finally {
       testDb.prepare('UPDATE addons SET enabled = 1 WHERE id = ?').run(ADDON_IDS.COLLECTIONS);

server/tests/unit/nest/collections.controller.test.ts

@@ -90,8 +90,8 @@ describe('CollectionsController', () => {
       expect(svc.setStatus).toHaveBeenCalledWith(1, 9, 'want', 'sid');
       expect(c.deletePlace(user, '9', 'sid')).toEqual({ success: true });
       expect(svc.deletePlace).toHaveBeenCalledWith(1, 9, 'sid');
-      c.saveFromTrip(user, { collection_id: 3, source_trip_id: 5, source_place_id: 8 } as never);
-      expect(svc.saveFromTripPlace).toHaveBeenCalledWith(1, 3, 5, 8, undefined);
+      c.saveFromTrip(user, { collection_id: 3, source_trip_id: 5, source_place_id: 8 } as never, 'sid');
+      expect(svc.saveFromTripPlace).toHaveBeenCalledWith(1, 3, 5, 8, undefined, 'sid');
       expect(c.copyToTrip(user, { trip_id: 5, place_ids: [9] } as never)).toEqual({ copied: 1, skipped: [] });
     });
 

server/tests/unit/nest/collections.service.test.ts

@@ -1,8 +1,9 @@
 /**
- * Unit tests for the DI-native CollectionsService (COLLECTIONS-SVC-001 … 081;
+ * Unit tests for the DI-native CollectionsService (COLLECTIONS-SVC-001 … 092;
  * moved 1:1 from the legacy tests/unit/services/collectionsService.test.ts,
  * case IDs preserved — the 080/081 membership-lookup cases are new with the
- * fold). Real in-memory SQLite (full schema + migrations) so the SQL —
+ * fold, and the 090–092 band pins the post-fold quirk fixes: all-or-nothing
+ * bulk writes and socket-id forwarding on the from-trip saves). Real in-memory SQLite (full schema + migrations) so the SQL —
  * owner/member visibility, the collection-scoped dedup, the fusion state
  * machine and the widened photo-cache reference check — is exercised
  * faithfully. Keeps its own clearCollections() reset (the shared
@@ -795,3 +796,59 @@ describe('membership lookups', () => {
     expect(svc.findMembershipForUser(member.id, col.id)).toEqual({ is_member: true, is_owner: false, status: 'accepted' });
   });
 });
+
+// ── Post-fold quirk fixes (the trailing fix(server) commit) ─────────────────
+
+describe('atomic bulk writes (post-fold quirk fixes)', () => {
+  it('COLLECTIONS-SVC-090: deletePlacesMany is all-or-nothing — a mid-list 403 deletes nothing', () => {
+    const u = createUser(testDb).user;
+    const otherOwner = createUser(testDb).user;
+    const mine = svc.createCollection(u.id, { name: 'Mine' });
+    const shared = svc.createCollection(otherOwner.id, { name: 'Shared' });
+    // u is an editor on the shared list — can add/edit but NOT delete (owner/admin only).
+    testDb.prepare("INSERT INTO collection_members (collection_id, user_id, status, role) VALUES (?, ?, 'accepted', 'editor')").run(shared.id, u.id);
+    const p1 = svc.savePlace(u.id, { collection_id: mine.id, name: 'Deletable' }).place!;
+    const p2 = svc.savePlace(u.id, { collection_id: shared.id, name: 'Protected' }).place!;
+
+    // The relocated legacy interleaved checks with deletes, so p1 was gone by
+    // the time p2's 403 fired. Now every id is checked first: nothing deleted.
+    expect(() => svc.deletePlacesMany(u.id, [p1.id, p2.id])).toThrow('Only an admin can delete places from this list');
+    expect(testDb.prepare('SELECT COUNT(*) n FROM collection_places WHERE id = ?').get(p1.id)).toEqual({ n: 1 });
+    expect(testDb.prepare('SELECT COUNT(*) n FROM collection_places WHERE id = ?').get(p2.id)).toEqual({ n: 1 });
+  });
+
+  it('COLLECTIONS-SVC-091: assignLabels permission-checks every list before writing anything', () => {
+    const u = createUser(testDb).user;
+    const otherOwner = createUser(testDb).user;
+    const mine = svc.createCollection(u.id, { name: 'Mine' });
+    const readonly = svc.createCollection(otherOwner.id, { name: 'ReadOnly' });
+    testDb.prepare("INSERT INTO collection_members (collection_id, user_id, status, role) VALUES (?, ?, 'accepted', 'viewer')").run(readonly.id, u.id);
+    const label = svc.createLabel(u.id, mine.id, 'Coast');
+    const pa = svc.savePlace(u.id, { collection_id: mine.id, name: 'A' }).place!;
+    const pb = testDb.prepare('INSERT INTO collection_places (collection_id, owner_id, saved_by, name) VALUES (?, ?, ?, ?)')
+      .run(readonly.id, otherOwner.id, otherOwner.id, 'B').lastInsertRowid as number;
+
+    // The relocated legacy checked per list inside the write loop, so `mine`
+    // was labeled before `readonly`'s 403 fired. Now all lists check first.
+    expect(() => svc.assignLabels(u.id, [label.id], [pa.id, Number(pb)], false)).toThrow('You have read-only access to this list');
+    expect(testDb.prepare('SELECT COUNT(*) n FROM collection_place_labels WHERE collection_place_id = ?').get(pa.id)).toEqual({ n: 0 });
+  });
+
+  it('COLLECTIONS-SVC-092: from-trip saves forward the socket id so the origin client does not echo', () => {
+    const u = createUser(testDb).user;
+    createCategory(testDb);
+    const trip = createTrip(testDb, u.id);
+    const place = createPlace(testDb, trip.id, { name: 'Louvre' });
+    // Distinct coords — the factory default would coord-dedup against Louvre.
+    const place2 = createPlace(testDb, trip.id, { name: 'Orsay', lat: 48.86, lng: 2.3266 });
+    const col = svc.createCollection(u.id, { name: 'From trip' });
+
+    broadcastToUser.mockClear();
+    svc.saveFromTripPlace(u.id, col.id, trip.id, place.id, undefined, 'sock-1');
+    expect(broadcastToUser).toHaveBeenCalledWith(u.id, expect.objectContaining({ type: 'collections:updated' }), 'sock-1');
+
+    broadcastToUser.mockClear();
+    svc.saveFromTripPlaces(u.id, col.id, trip.id, [place2.id], undefined, 'sock-2');
+    expect(broadcastToUser).toHaveBeenCalledWith(u.id, expect.objectContaining({ type: 'collections:updated' }), 'sock-2');
+  });
+});