From c78134831fc992bda1507404eec3585868d7afdf Mon Sep 17 00:00:00 2001 From: jubnl Date: Sat, 1 Aug 2026 14:03:05 +0200 Subject: [PATCH] fix(server): tidy collections quirks after the DI fold --- migration-graph.md | 43 +++- server/src/nest/README.md | 14 +- .../collections/collections.controller.ts | 8 +- .../src/nest/collections/collections.mcp.ts | 59 ++++- .../nest/collections/collections.service.ts | 230 ++++++++++-------- .../tests/unit/mcp/tools-collections.test.ts | 22 +- .../unit/nest/collections.controller.test.ts | 4 +- .../unit/nest/collections.service.test.ts | 61 ++++- 8 files changed, 310 insertions(+), 131 deletions(-) diff --git a/migration-graph.md b/migration-graph.md index 1cb069020..18d8a334e 100644 --- a/migration-graph.md +++ b/migration-graph.md @@ -483,8 +483,47 @@ Wave-2 `permissions` + `auditLog` pair were the first frontier picks — all don 23-case `tools-collections` characterization suite; and (b) the legacy registrar registered **without an addon gate** while REST and the plugin host both gate on the collections addon — a real behavioral asymmetry that a naive "add `when:` like todo" - port would have silently fixed; parity kept it and the new suite pins it (a quirk-fix - candidate for a trailing commit, not part of the relocation). + port would have silently fixed; parity kept it and the new suite pins it (fixed in the + trailing quirk commit below). + +## Quirks fixed after the collections fold (the trailing `fix(server)` commit) + +The relocation itself was byte-identical; these were then fixed on top, each with a +regression test, so the parity diff and the behaviour change stayed in separate commits: + +1. **No transactions anywhere.** Every multi-statement write (savePlace + tags + + ratings-copy, saveFromTripPlaces, updatePlace's field/tag/label rewrite, + deletePlacesMany, copyToTrip's places/tags/ratings inserts, assignLabels, + reorderCollections) ran statement-by-statement. All now run in `db.transaction()` + ("Transactions are not optional", server/CLAUDE.md). +2. **Bulk ops could partially commit.** `deletePlacesMany` interleaved + `assertCanDelete` with the deletes and `assignLabels` checked each list inside its + write loop — a mid-list 403/404 left earlier work committed. Both now permission-check + every id/list up front and write all-or-nothing (COLLECTIONS-SVC-090/091). +3. **The MCP tools ignored the collections addon.** The legacy registrar registered its + 25 tools unconditionally while REST (`CollectionsAddonGuard`) and the plugin host + (`requireAddon`) refuse when the addon is off — and the addon is seeded **disabled** + by default, so a fresh install exposed the whole MCP surface for a feature the admin + never turned on. Every entry now rides `when: collectionsAddonOn` (todo pattern); + the characterization case flipped from pinning the absence to pinning the gate. +4. **The from-trip saves echoed to their origin client.** The client sends + `X-Socket-Id` on every request, but `POST /places/from-trip`(`-many`) never read the + header and `saveFromTripPlaces` hardcoded `socketId: undefined`, so the saving client + re-received its own `collections:updated`. Both routes now forward the header + (COLLECTIONS-SVC-092). +5. **Two MCP handlers let unexpected throws escape.** `invite_to_collection` and + `accept_collection_invite` were the only handlers without try/catch — a db-level + throw surfaced as an SDK protocol error instead of `isError` text. Both now catch. + +**Quirks deliberately preserved** (parity, not oversights): `setRating` gates on +`assertAccess`, not `assertCanEdit` — viewers cast personal votes by design (#1435); +`sendInvite`'s notification send stays fire-and-forget with its double `.catch(() => {})`; +`roleOf` collapses unknown member roles to `'editor'` (defensive default); +`updateCollection`/`updateLabel` still broadcast even when no field changed (a harmless +refresh); `updateLabel` sets no `updated_at` because `collection_labels` has no such +column (not a defect); `reorderCollections` and `copyToTrip` still broadcast nothing; +`findMembership` still refuses to match on a bare name (deliberate, anti-false-positive); +and the dead `findMembershipForUser` export stays (now tested, zero consumers). ## Quirks fixed after the place fold (the trailing `fix(server)` commit) diff --git a/server/src/nest/README.md b/server/src/nest/README.md index ba9f0ea53..49e99f9b7 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -298,10 +298,16 @@ exports into `CollectionsService` over DatabaseService + PermissionsService + RealtimeService, with the `deleteOldCollectionCover` path re-anchored one directory deeper and the `sendInvite` lazy notificationService `import()` kept, collab precedent; the 25-tool legacy registrar `mcp/tools/collections.ts` -moved to `collections.mcp.ts` — deliberately with NO `when:` addon gate, since -the legacy registrar registered unconditionally while REST/plugin-host gate on -the addon, a parity quirk now pinned by a new 23-case `tools-collections` -characterization suite (the legacy registrar had no tool-level tests at all); +moved to `collections.mcp.ts` — at relocation time deliberately with NO +`when:` addon gate, since the legacy registrar registered unconditionally +while REST/plugin-host gate on the addon; the trailing `fix(server)` quirk +commit then gated all 25 tools (the addon ships disabled by default, so the +ungated surface was live on fresh installs) alongside wrapping every +multi-statement write in `db.transaction()`, making the bulk ops +all-or-nothing, and forwarding `X-Socket-Id` on the from-trip saves — each +pinned by a new 23-case `tools-collections` characterization suite plus the +`COLLECTIONS-SVC-090…092` band (the legacy registrar had no tool-level tests +at all); the plugin host swapped its 7 collections imports for the injected service — its 22nd constructor dep — and NO bridge was needed anywhere; the dead `buildDedupSet` module helper was dropped in the move, the only line that diff --git a/server/src/nest/collections/collections.controller.ts b/server/src/nest/collections/collections.controller.ts index 450e26bdf..45d23955c 100644 --- a/server/src/nest/collections/collections.controller.ts +++ b/server/src/nest/collections/collections.controller.ts @@ -113,14 +113,14 @@ export class CollectionsController { @Post('places/from-trip') @HttpCode(200) - saveFromTrip(@CurrentUser() user: User, @Body() body: CollectionSaveFromTripDto) { - return this.collections.saveFromTripPlace(user.id, body.collection_id, body.source_trip_id, body.source_place_id, body.force); + saveFromTrip(@CurrentUser() user: User, @Body() body: CollectionSaveFromTripDto, @Headers('x-socket-id') socketId?: string) { + return this.collections.saveFromTripPlace(user.id, body.collection_id, body.source_trip_id, body.source_place_id, body.force, socketId); } @Post('places/from-trip-many') @HttpCode(200) - saveFromTripMany(@CurrentUser() user: User, @Body() body: CollectionSaveFromTripManyDto) { - return this.collections.saveFromTripPlaces(user.id, body.collection_id, body.source_trip_id, body.source_place_ids, body.force); + saveFromTripMany(@CurrentUser() user: User, @Body() body: CollectionSaveFromTripManyDto, @Headers('x-socket-id') socketId?: string) { + return this.collections.saveFromTripPlaces(user.id, body.collection_id, body.source_trip_id, body.source_place_ids, body.force, socketId); } @Post('places/delete-many') diff --git a/server/src/nest/collections/collections.mcp.ts b/server/src/nest/collections/collections.mcp.ts index 8b9ee1abc..f90a32d3f 100644 --- a/server/src/nest/collections/collections.mcp.ts +++ b/server/src/nest/collections/collections.mcp.ts @@ -6,6 +6,8 @@ import { } from '@trek/nest-mcp'; import { z } from 'zod'; import { isDemoUser } from '../../services/authService'; +import { isAddonEnabled } from '../addons/addons.bridge'; +import { ADDON_IDS } from '../../addons'; import { DatabaseService } from '../database/database.service'; import { CollectionsService } from './collections.service'; import { @@ -32,6 +34,11 @@ function fail(err: unknown) { return { content: [{ type: 'text' as const, text: errText(err) }], isError: true }; } +/** Post-fold quirk fix: the legacy registrar registered unconditionally while + * REST (CollectionsAddonGuard) and the plugin host (requireAddon) gate on the + * collections addon — the decorator port now gates too. */ +const collectionsAddonOn = () => isAddonEnabled(ADDON_IDS.COLLECTIONS); + /** * Collections MCP surface (#1435) — ported 1:1 from the legacy registrar * src/mcp/tools/collections.ts: the full set of actions a member can take on @@ -40,9 +47,9 @@ function fail(err: unknown) { * names, descriptions, schemas, annotations and error/payload shapes. Every * service call enforces the caller's membership/role, so the tools stay thin * wrappers. The legacy `if (R)` / `if (W)` scope checks map to the declarative - * collections read/write access markers (resolved by trekMcpAccessPolicy); - * like the legacy registrar there is deliberately NO addon `when:` gate — the - * REST surface is addon-gated but the MCP tools never were (parity). + * collections read/write access markers (resolved by trekMcpAccessPolicy), + * plus the `when:` collections-addon gate the legacy registrar lacked (fixed + * in the trailing quirk commit — REST and the plugin host always gated). */ @McpController() export class CollectionsMcp { @@ -62,6 +69,7 @@ export class CollectionsMcp { description: 'List all saved-place collections (lists) the user owns or has accepted a share for, plus any pending incoming invites. Use get_collection for a list\'s places.', inputSchema: {}, annotations: TOOL_ANNOTATIONS_READONLY, + when: collectionsAddonOn, access: { group: 'collections', mode: 'read' }, }) async listCollections(_args: Record, ctx: McpContext) { @@ -73,6 +81,7 @@ export class CollectionsMcp { description: 'Get one collection with its members, labels, and all saved places. Each place includes rating_avg / rating_count and the per-member ratings (#1435) so you can plan around highly-rated spots.', inputSchema: { collectionId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_READONLY, + when: collectionsAddonOn, access: { group: 'collections', mode: 'read' }, }) async getCollection({ collectionId }: { collectionId: number }, ctx: McpContext) { @@ -84,6 +93,7 @@ export class CollectionsMcp { description: 'List users who can still be invited to a collection (excludes current members and guests). Use the returned ids with invite_to_collection.', inputSchema: { collectionId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_READONLY, + when: collectionsAddonOn, access: { group: 'collections', mode: 'read' }, }) async availableCollectionUsers({ collectionId }: { collectionId: number }, ctx: McpContext) { @@ -105,6 +115,7 @@ export class CollectionsMcp { description: 'Create a new saved-place collection (list) owned by the user.', inputSchema: collectionCreateRequestSchema.shape, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async createCollection(body: CollectionCreateRequest, ctx: McpContext) { @@ -117,6 +128,7 @@ export class CollectionsMcp { description: 'Update a collection\'s name, description, colour, icon, cover, links, or sort order. Owner/admin only.', inputSchema: { collectionId: z.number().int().positive(), ...collectionUpdateRequestSchema.shape }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async updateCollection({ collectionId, ...body }: { collectionId: number } & CollectionUpdateRequest, ctx: McpContext) { @@ -129,6 +141,7 @@ export class CollectionsMcp { description: 'Permanently delete a collection and all its saved places. Owner only. This cannot be undone.', inputSchema: { collectionId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_DELETE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async deleteCollection({ collectionId }: { collectionId: number }, ctx: McpContext) { @@ -141,6 +154,7 @@ export class CollectionsMcp { description: 'Reorder the user\'s collections. Pass every collection id in the desired order.', inputSchema: { orderedIds: z.array(z.number().int().positive()).min(1) }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async reorderCollections({ orderedIds }: { orderedIds: number[] }, ctx: McpContext) { @@ -155,6 +169,7 @@ export class CollectionsMcp { description: 'Save a place into a collection from a raw payload (name required; set google_place_id/osm_id from search_place for rich details). Returns a duplicate marker instead of saving when a similar place already exists, unless force is true.', inputSchema: collectionSavePlaceRequestSchema.shape, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async savePlaceToCollection(body: CollectionSavePlaceRequest, ctx: McpContext) { @@ -172,6 +187,7 @@ export class CollectionsMcp { force: z.boolean().optional(), }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async saveTripPlacesToCollection( @@ -187,6 +203,7 @@ export class CollectionsMcp { description: 'Update a saved place\'s name, description, notes, status, category, links, tags, labels, image, or move it to another collection (set collection_id).', inputSchema: { placeId: z.number().int().positive(), ...collectionPlaceUpdateRequestSchema.shape }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async updateCollectionPlace({ placeId, ...body }: { placeId: number } & CollectionPlaceUpdateRequest, ctx: McpContext) { @@ -199,6 +216,7 @@ export class CollectionsMcp { description: 'Set a saved place\'s status: idea, want, or visited.', inputSchema: { placeId: z.number().int().positive(), status: z.enum(COLLECTION_STATUSES) }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async setCollectionPlaceStatus({ placeId, status }: { placeId: number; status: CollectionStatus }, ctx: McpContext) { @@ -214,6 +232,7 @@ export class CollectionsMcp { rating: z.number().int().min(1).max(5).nullable().optional().describe('1-5 stars; null/omitted clears the vote'), }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async rateCollectionPlace({ placeId, rating }: { placeId: number; rating?: number | null }, ctx: McpContext) { @@ -226,6 +245,7 @@ export class CollectionsMcp { description: 'Remove a saved place from its collection. Requires delete permission on the list.', inputSchema: { placeId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_DELETE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async deleteCollectionPlace({ placeId }: { placeId: number }, ctx: McpContext) { @@ -238,6 +258,7 @@ export class CollectionsMcp { description: 'Copy one or more saved collection places into a trip (dedup precheck on the server). Ratings (#1435) travel into the trip; trip members keep voting there. Requires edit access to the target trip.', inputSchema: collectionCopyToTripRequestSchema.shape, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async copyCollectionPlacesToTrip(body: CollectionCopyToTripRequest, ctx: McpContext) { @@ -252,6 +273,7 @@ export class CollectionsMcp { description: 'Create a custom per-collection label (name + optional hex colour) for grouping/filtering places.', inputSchema: collectionLabelCreateRequestSchema.shape, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async createCollectionLabel({ collection_id, name, color }: CollectionLabelCreateRequest, ctx: McpContext) { @@ -264,6 +286,7 @@ export class CollectionsMcp { description: 'Rename or recolour a collection label, or change its sort order.', inputSchema: { labelId: z.number().int().positive(), ...collectionLabelUpdateRequestSchema.shape }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async updateCollectionLabel({ labelId, ...body }: { labelId: number } & CollectionLabelUpdateRequest, ctx: McpContext) { @@ -276,6 +299,7 @@ export class CollectionsMcp { description: 'Delete a collection label; its assignments on places are cleared.', inputSchema: { labelId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_DELETE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async deleteCollectionLabel({ labelId }: { labelId: number }, ctx: McpContext) { @@ -288,6 +312,7 @@ export class CollectionsMcp { description: 'Add (or with remove=true, take away) one or more labels across a set of saved places. Only labels belonging to each place\'s own list are applied.', inputSchema: { ...collectionLabelAssignRequestSchema.shape, remove: z.boolean().optional() }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async assignCollectionLabels( @@ -305,14 +330,19 @@ export class CollectionsMcp { description: 'Invite a user (by id, from available_collection_users) to collaborate on a collection, with a role of viewer, editor, or admin (default editor). Owner only.', inputSchema: collectionInviteRequestSchema.shape, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async inviteToCollection({ collection_id, user_id, role }: CollectionInviteRequest, ctx: McpContext) { const demo = this.denyDemo(ctx.userId); if (demo) return demo; - const me = this.db.get<{ username: string; email: string }>('SELECT username, email FROM users WHERE id = ?', ctx.userId); - const res = this.collections.sendInvite(collection_id, ctx.userId, me?.username ?? '', me?.email ?? '', user_id, role); - if (res.error) return { content: [{ type: 'text' as const, text: res.error }], isError: true }; - return ok({ success: true }); + // try/catch added with the post-fold quirk pass — the legacy handler was one + // of two where an unexpected throw escaped to the SDK instead of isError. + try { + const me = this.db.get<{ username: string; email: string }>('SELECT username, email FROM users WHERE id = ?', ctx.userId); + const res = this.collections.sendInvite(collection_id, ctx.userId, me?.username ?? '', me?.email ?? '', user_id, role); + if (res.error) return { content: [{ type: 'text' as const, text: res.error }], isError: true }; + return ok({ success: true }); + } catch (err) { return fail(err); } } @Tool({ @@ -324,6 +354,7 @@ export class CollectionsMcp { role: z.enum(COLLECTION_ROLES), }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async setCollectionMemberRole( @@ -339,6 +370,7 @@ export class CollectionsMcp { description: 'Remove an accepted member from a shared collection (a kick). Owner only.', inputSchema: { collectionId: z.number().int().positive(), userId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_DELETE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async removeCollectionMember( @@ -354,6 +386,7 @@ export class CollectionsMcp { description: 'Cancel a pending invite you sent to a user for a collection. Owner only.', inputSchema: { collectionId: z.number().int().positive(), userId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async cancelCollectionInvite( @@ -369,13 +402,17 @@ export class CollectionsMcp { description: 'Accept a pending invite to join a shared collection.', inputSchema: { collectionId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async acceptCollectionInvite({ collectionId }: { collectionId: number }, ctx: McpContext) { const demo = this.denyDemo(ctx.userId); if (demo) return demo; - const res = this.collections.acceptInvite(ctx.userId, collectionId, undefined); - if (res.error) return { content: [{ type: 'text' as const, text: res.error }], isError: true }; - return ok({ success: true }); + // try/catch added with the post-fold quirk pass (see inviteToCollection). + try { + const res = this.collections.acceptInvite(ctx.userId, collectionId, undefined); + if (res.error) return { content: [{ type: 'text' as const, text: res.error }], isError: true }; + return ok({ success: true }); + } catch (err) { return fail(err); } } @Tool({ @@ -383,6 +420,7 @@ export class CollectionsMcp { description: 'Decline a pending invite to a shared collection.', inputSchema: { collectionId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async declineCollectionInvite({ collectionId }: { collectionId: number }, ctx: McpContext) { @@ -395,6 +433,7 @@ export class CollectionsMcp { description: 'Leave a shared collection you are a member of. The owner cannot leave (delete the list instead).', inputSchema: { collectionId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_WRITE, + when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, }) async leaveCollection({ collectionId }: { collectionId: number }, ctx: McpContext) { diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index 9e511ae0a..b0bda4b06 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -418,8 +418,10 @@ export class CollectionsService { reorderCollections(userId: number, orderedIds: number[]): void { const visible = new Set(this.accessibleCollectionIds(userId)); const stmt = this.db.prepare('UPDATE collections SET sort_order = ? WHERE id = ?'); - orderedIds.forEach((cid, index) => { - if (visible.has(cid)) stmt.run(index, cid); + this.db.transaction(() => { + orderedIds.forEach((cid, index) => { + if (visible.has(cid)) stmt.run(index, cid); + }); }); } @@ -492,42 +494,47 @@ export class CollectionsService { } const ownerId = this.ownerOf(body.collection_id); - const result = this.db.run(` + // Insert + tags + ratings-copy are one logical write — atomic since the + // post-fold quirk pass (the relocation carried them un-transacted). + const placeId = this.db.transaction(() => { + const result = this.db.run(` INSERT INTO collection_places ( collection_id, owner_id, saved_by, name, description, lat, lng, address, category_id, price, currency, notes, image_url, google_place_id, google_ftid, osm_id, website, phone, status, source_trip_id, source_place_id, links ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `, - body.collection_id, ownerId, userId, - body.name, body.description ?? null, body.lat ?? null, body.lng ?? null, body.address ?? null, - body.category_id ?? null, body.price ?? null, body.currency ?? null, body.notes ?? null, - body.image_url ?? null, body.google_place_id ?? null, body.google_ftid ?? null, - body.osm_id ?? null, body.website ?? null, body.phone ?? null, - body.status ?? 'idea', body.source_trip_id ?? null, body.source_place_id ?? null, - serializeLinks(body.links), - ); + body.collection_id, ownerId, userId, + body.name, body.description ?? null, body.lat ?? null, body.lng ?? null, body.address ?? null, + body.category_id ?? null, body.price ?? null, body.currency ?? null, body.notes ?? null, + body.image_url ?? null, body.google_place_id ?? null, body.google_ftid ?? null, + body.osm_id ?? null, body.website ?? null, body.phone ?? null, + body.status ?? 'idea', body.source_trip_id ?? null, body.source_place_id ?? null, + serializeLinks(body.links), + ); - const placeId = Number(result.lastInsertRowid); - this.attachTags(placeId, body.tag_ids); - // Carry trip ratings ONLY when the caller can actually see the source place. - // source_place_id/source_trip_id are raw client input, so verify trip access + - // that the place lives in that trip before reading place_ratings — otherwise a - // member could harvest co-members' votes on places in trips they cannot access - // (mirrors the canAccessTrip gate in saveFromTripPlace). - if ( - body.source_place_id && body.source_trip_id && - this.db.canAccessTrip(body.source_trip_id, userId) && - this.db.get('SELECT 1 FROM places WHERE id = ? AND trip_id = ?', body.source_place_id, body.source_trip_id) - ) { - this.copyTripRatings(body.source_place_id, placeId, body.collection_id); - } + const id = Number(result.lastInsertRowid); + this.attachTags(id, body.tag_ids); + // Carry trip ratings ONLY when the caller can actually see the source place. + // source_place_id/source_trip_id are raw client input, so verify trip access + + // that the place lives in that trip before reading place_ratings — otherwise a + // member could harvest co-members' votes on places in trips they cannot access + // (mirrors the canAccessTrip gate in saveFromTripPlace). + if ( + body.source_place_id && body.source_trip_id && + this.db.canAccessTrip(body.source_trip_id, userId) && + this.db.get('SELECT 1 FROM places WHERE id = ? AND trip_id = ?', body.source_place_id, body.source_trip_id) + ) { + this.copyTripRatings(body.source_place_id, id, body.collection_id); + } + return id; + }); this.notifyCollectionUsers(body.collection_id, socketId, 'collections:updated'); return { place: this.getPlaceById(placeId) }; } saveFromTripPlace( - userId: number, collectionId: number, tripId: number, placeId: number, force?: boolean, + userId: number, collectionId: number, tripId: number, placeId: number, force?: boolean, socketId?: string, ): CollectionSaveResult { this.assertCanEdit(userId, collectionId); if (!this.db.canAccessTrip(tripId, userId)) httpError(404, 'Trip not found'); @@ -555,14 +562,14 @@ export class CollectionsService { source_trip_id: tripId, source_place_id: placeId, force, - }); + }, socketId); } /** Bulk copy of several trip places into a list in one shot — one access check, * one WS notify (vs saving each place individually). Mirrors saveFromTripPlace's * field mapping + dedup; skips duplicates unless force. Status starts at 'idea'. */ saveFromTripPlaces( - userId: number, collectionId: number, tripId: number, placeIds: number[], force?: boolean, + userId: number, collectionId: number, tripId: number, placeIds: number[], force?: boolean, socketId?: string, ): { copied: number; skipped: { id: number; name: string }[] } { this.assertCanEdit(userId, collectionId); if (!this.db.canAccessTrip(tripId, userId)) httpError(404, 'Trip not found'); @@ -577,28 +584,31 @@ export class CollectionsService { `); let copied = 0; const skipped: { id: number; name: string }[] = []; - for (const placeId of placeIds) { - const p = this.db.get>('SELECT * FROM places WHERE id = ? AND trip_id = ?', placeId, tripId); - if (!p) continue; - const name = p.name as string; - const lat = (p.lat as number | null) ?? null; - const lng = (p.lng as number | null) ?? null; - if (!force && this.findDuplicateCollectionPlace(collectionId, { name, lat, lng })) { - skipped.push({ id: placeId, name }); - continue; + // The whole batch is one logical write — atomic since the post-fold quirk pass. + this.db.transaction(() => { + for (const placeId of placeIds) { + const p = this.db.get>('SELECT * FROM places WHERE id = ? AND trip_id = ?', placeId, tripId); + if (!p) continue; + const name = p.name as string; + const lat = (p.lat as number | null) ?? null; + const lng = (p.lng as number | null) ?? null; + if (!force && this.findDuplicateCollectionPlace(collectionId, { name, lat, lng })) { + skipped.push({ id: placeId, name }); + continue; + } + const res = insert.run( + collectionId, ownerId, userId, + name, (p.description as string | null) ?? null, lat, lng, (p.address as string | null) ?? null, + (p.category_id as number | null) ?? null, (p.price as number | null) ?? null, (p.currency as string | null) ?? null, (p.notes as string | null) ?? null, + (p.image_url as string | null) ?? null, (p.google_place_id as string | null) ?? null, (p.google_ftid as string | null) ?? null, + (p.osm_id as string | null) ?? null, (p.website as string | null) ?? null, (p.phone as string | null) ?? null, + tripId, placeId, + ); + this.copyTripRatings(placeId, Number(res.lastInsertRowid), collectionId); + copied++; } - const res = insert.run( - collectionId, ownerId, userId, - name, (p.description as string | null) ?? null, lat, lng, (p.address as string | null) ?? null, - (p.category_id as number | null) ?? null, (p.price as number | null) ?? null, (p.currency as string | null) ?? null, (p.notes as string | null) ?? null, - (p.image_url as string | null) ?? null, (p.google_place_id as string | null) ?? null, (p.google_ftid as string | null) ?? null, - (p.osm_id as string | null) ?? null, (p.website as string | null) ?? null, (p.phone as string | null) ?? null, - tripId, placeId, - ); - this.copyTripRatings(placeId, Number(res.lastInsertRowid), collectionId); - copied++; - } - if (copied > 0) this.notifyCollectionUsers(collectionId, undefined, 'collections:updated'); + }); + if (copied > 0) this.notifyCollectionUsers(collectionId, socketId, 'collections:updated'); return { copied, skipped }; } @@ -632,21 +642,25 @@ export class CollectionsService { movedTo = body.collection_id; } - if (updates.length > 0) { - updates.push("updated_at = CURRENT_TIMESTAMP"); - params.push(placeId); - this.db.run(`UPDATE collection_places SET ${updates.join(', ')} WHERE id = ?`, ...params); - } + // Field update + tag rewrite + label rewrite are one logical write — atomic + // since the post-fold quirk pass. + this.db.transaction(() => { + if (updates.length > 0) { + updates.push("updated_at = CURRENT_TIMESTAMP"); + params.push(placeId); + this.db.run(`UPDATE collection_places SET ${updates.join(', ')} WHERE id = ?`, ...params); + } - if (body.tag_ids !== undefined) { - this.db.run('DELETE FROM collection_place_tags WHERE collection_place_id = ?', placeId); - this.attachTags(placeId, body.tag_ids); - } + if (body.tag_ids !== undefined) { + this.db.run('DELETE FROM collection_place_tags WHERE collection_place_id = ?', placeId); + this.attachTags(placeId, body.tag_ids); + } - // Labels are collection-scoped: a move invalidates the source list's labels; - // a provided label_ids set replaces them against the (target) collection. - if (movedTo) this.db.run('DELETE FROM collection_place_labels WHERE collection_place_id = ?', placeId); - if (body.label_ids !== undefined) this.setPlaceLabels(placeId, movedTo ?? currentCollection, body.label_ids); + // Labels are collection-scoped: a move invalidates the source list's labels; + // a provided label_ids set replaces them against the (target) collection. + if (movedTo) this.db.run('DELETE FROM collection_place_labels WHERE collection_place_id = ?', placeId); + if (body.label_ids !== undefined) this.setPlaceLabels(placeId, movedTo ?? currentCollection, body.label_ids); + }); if (body.image_url !== undefined && prevImage !== (body.image_url ?? null)) { reclaimPlaceImage(prevImage); @@ -695,6 +709,10 @@ export class CollectionsService { } deletePlacesMany(userId: number, ids: number[], socketId?: string): number[] { + // All-or-nothing since the post-fold quirk pass: every id is resolved and + // permission-checked BEFORE any delete (the relocated legacy interleaved + // checks with deletes, so a mid-list 403/404 left earlier deletes committed), + // and the deletes then run in one transaction. const deleted: number[] = []; const touched = new Set(); const images: (string | null)[] = []; @@ -702,10 +720,14 @@ export class CollectionsService { const collectionId = this.collectionIdOfPlace(id); this.assertCanDelete(userId, collectionId); images.push(this.db.get<{ image_url: string | null }>('SELECT image_url FROM collection_places WHERE id = ?', id)?.image_url ?? null); - this.db.run('DELETE FROM collection_places WHERE id = ?', id); - deleted.push(id); touched.add(collectionId); } + this.db.transaction(() => { + for (const id of ids) { + this.db.run('DELETE FROM collection_places WHERE id = ?', id); + deleted.push(id); + } + }); for (const image of images) reclaimPlaceImage(image); touched.forEach(cid => this.notifyCollectionUsers(cid, socketId, 'collections:updated')); return deleted; @@ -776,28 +798,31 @@ export class CollectionsService { let copied = 0; const skipped: { id: number; name: string }[] = []; - for (const s of sources) { - if (!body.force && isCollectionPlaceDuplicate({ name: s.name, lat: s.lat, lng: s.lng }, dedup)) { - skipped.push({ id: s.id, name: s.name }); - continue; - } - const res = insertPlace.run( - body.trip_id, s.name, s.description, s.lat, s.lng, s.address, s.category_id, s.price, - s.currency, s.notes, s.image_url, s.google_place_id, s.google_ftid, s.website, s.phone, s.osm_id, - ); - const newPlaceId = Number(res.lastInsertRowid); - const tagIds = this.db.all<{ tag_id: number }>('SELECT tag_id FROM collection_place_tags WHERE collection_place_id = ?', s.id); - for (const t of tagIds) insertTag.run(newPlaceId, t.tag_id); - // Ratings travel into the trip too (#1435), but only for trip members — a - // collection voter who isn't on the trip stays out of it. Trip members keep - // voting there; nothing is mirrored back. - const votes = this.db.all<{ user_id: number; rating: number }>('SELECT user_id, rating FROM collection_place_ratings WHERE collection_place_id = ?', s.id); - for (const v of votes) if (tripMemberIds.has(v.user_id)) insertRating.run(newPlaceId, v.user_id, v.rating); + // The whole copy is one logical write — atomic since the post-fold quirk pass. + this.db.transaction(() => { + for (const s of sources) { + if (!body.force && isCollectionPlaceDuplicate({ name: s.name, lat: s.lat, lng: s.lng }, dedup)) { + skipped.push({ id: s.id, name: s.name }); + continue; + } + const res = insertPlace.run( + body.trip_id, s.name, s.description, s.lat, s.lng, s.address, s.category_id, s.price, + s.currency, s.notes, s.image_url, s.google_place_id, s.google_ftid, s.website, s.phone, s.osm_id, + ); + const newPlaceId = Number(res.lastInsertRowid); + const tagIds = this.db.all<{ tag_id: number }>('SELECT tag_id FROM collection_place_tags WHERE collection_place_id = ?', s.id); + for (const t of tagIds) insertTag.run(newPlaceId, t.tag_id); + // Ratings travel into the trip too (#1435), but only for trip members — a + // collection voter who isn't on the trip stays out of it. Trip members keep + // voting there; nothing is mirrored back. + const votes = this.db.all<{ user_id: number; rating: number }>('SELECT user_id, rating FROM collection_place_ratings WHERE collection_place_id = ?', s.id); + for (const v of votes) if (tripMemberIds.has(v.user_id)) insertRating.run(newPlaceId, v.user_id, v.rating); - if (s.name) dedup.names.add(s.name.trim().toLowerCase()); - else if (s.lat != null && s.lng != null) dedup.coords.push({ lat: s.lat, lng: s.lng }); - copied++; - } + if (s.name) dedup.names.add(s.name.trim().toLowerCase()); + else if (s.lat != null && s.lng != null) dedup.coords.push({ lat: s.lat, lng: s.lng }); + copied++; + } + }); return { copied, skipped }; } @@ -936,21 +961,30 @@ export class CollectionsService { if (!byCollection.has(cid)) byCollection.set(cid, []); byCollection.get(cid)!.push(pid); } + // All-or-nothing since the post-fold quirk pass: every touched list is + // permission-checked BEFORE any write (the relocated legacy checked inside + // the write loop, so a later list's 403 left earlier lists modified), and + // the writes then run in one transaction. Broadcasts still skip lists where + // no provided label applied. + for (const cid of byCollection.keys()) this.assertCanEdit(userId, cid); let changed = 0; - for (const [cid, pids] of byCollection) { - this.assertCanEdit(userId, cid); - const valid = new Set(this.loadLabelsByCollection(cid).map(l => l.id)); - const applicable = labelIds.filter(id => valid.has(id)); - if (applicable.length === 0) continue; - if (remove) { - const del = this.db.prepare('DELETE FROM collection_place_labels WHERE collection_place_id = ? AND label_id = ?'); - for (const pid of pids) for (const lid of applicable) changed += del.run(pid, lid).changes; - } else { - const ins = this.db.prepare('INSERT OR IGNORE INTO collection_place_labels (collection_place_id, label_id) VALUES (?, ?)'); - for (const pid of pids) for (const lid of applicable) changed += ins.run(pid, lid).changes; + const notified: number[] = []; + this.db.transaction(() => { + for (const [cid, pids] of byCollection) { + const valid = new Set(this.loadLabelsByCollection(cid).map(l => l.id)); + const applicable = labelIds.filter(id => valid.has(id)); + if (applicable.length === 0) continue; + if (remove) { + const del = this.db.prepare('DELETE FROM collection_place_labels WHERE collection_place_id = ? AND label_id = ?'); + for (const pid of pids) for (const lid of applicable) changed += del.run(pid, lid).changes; + } else { + const ins = this.db.prepare('INSERT OR IGNORE INTO collection_place_labels (collection_place_id, label_id) VALUES (?, ?)'); + for (const pid of pids) for (const lid of applicable) changed += ins.run(pid, lid).changes; + } + notified.push(cid); } - this.notifyCollectionUsers(cid, socketId, 'collections:updated'); - } + }); + for (const cid of notified) this.notifyCollectionUsers(cid, socketId, 'collections:updated'); return { changed }; } diff --git a/server/tests/unit/mcp/tools-collections.test.ts b/server/tests/unit/mcp/tools-collections.test.ts index 4ace10860..1e0ec358d 100644 --- a/server/tests/unit/mcp/tools-collections.test.ts +++ b/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); diff --git a/server/tests/unit/nest/collections.controller.test.ts b/server/tests/unit/nest/collections.controller.test.ts index fce83eca0..51fb461b6 100644 --- a/server/tests/unit/nest/collections.controller.test.ts +++ b/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: [] }); }); diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index 63fbc2a9d..805b8ec0f 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/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'); + }); +});