From 44c2573abb243ac7378df242f2d7fadb70b328ba Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 28 Aug 2026 13:07:41 +0200 Subject: [PATCH] feat(mcp): trip length, reminders, cover search, and the member permission A dateless trip made by a tool silently got seven days, and reminder_days was stuck on the default of three with no way to turn it off. Cover images had no search at all. add_trip_member and remove_trip_member hardcoded an owner check while REST consults the configurable permission matrix and the plugin RPC does too: three surfaces, two behaviours. They now use the same permission call update_trip already made, so an instance that lowered member_manage behaves consistently. The guest tools stay owner-only, because their REST routes are owner-only by design and document it. --- server/src/nest/trips/trips.mcp.ts | 108 ++++++-- .../tests/unit/mcp/tools-trip-members.test.ts | 244 +++++++++++++++++- server/tests/unit/mcp/tools-trips.test.ts | 229 +++++++++++++++- 3 files changed, 556 insertions(+), 25 deletions(-) diff --git a/server/src/nest/trips/trips.mcp.ts b/server/src/nest/trips/trips.mcp.ts index 62939f95b..3340f5ae0 100644 --- a/server/src/nest/trips/trips.mcp.ts +++ b/server/src/nest/trips/trips.mcp.ts @@ -1,8 +1,8 @@ import { McpController, Tool, Resource, ResourceTemplate, Prompt, type McpContext, - TOOL_ANNOTATIONS_READONLY, TOOL_ANNOTATIONS_WRITE, + TOOL_ANNOTATIONS_READONLY, TOOL_ANNOTATIONS_WRITE, TOOL_ANNOTATIONS_OPEN_WORLD_READONLY, TOOL_ANNOTATIONS_DELETE, TOOL_ANNOTATIONS_NON_IDEMPOTENT, - demoDenied, ok, + demoDenied, errorResult, ok, } from '../../nest-mcp'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; import { z } from 'zod'; @@ -13,7 +13,7 @@ import { TripReadModelService } from '../trip-read-model/trip-read-model.service import { ADDON_IDS } from '../../addons'; import { MAX_MCP_TRIP_DAYS, noAccess, permissionDenied } from '../../mcp/tools/_shared'; import { canRead, canReadTrips, canDeleteTrips } from '../../mcp/scopes'; -import { TripsService, NotFoundError, ValidationError } from './trips.service'; +import { TripsService, MAX_TRIP_DAYS, NotFoundError, ValidationError } from './trips.service'; import { TodoService } from '../todo/todo.service'; import { CollabService } from '../collab/collab.service'; import { AddonsService } from '../addons/addons.service'; @@ -87,13 +87,18 @@ export class TripsMcp { start_date: z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional().describe('Start date (YYYY-MM-DD)'), end_date: z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional().describe('End date (YYYY-MM-DD)'), currency: z.string().length(3).optional().describe('Currency code (e.g. EUR, USD)'), + day_count: z.number().int().min(1).max(MAX_TRIP_DAYS).optional().describe( + 'How many days a trip without dates gets (default 7). Ignored when start_date and end_date are both set, because the range decides the count.'), + reminder_days: z.number().int().min(0).max(30).optional().describe( + 'How many days before departure the pre-trip reminder fires (default 3). 0 turns the reminder off.'), }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, access: { group: 'trips', mode: 'write' }, }) async createTrip( - { title, description, start_date, end_date, currency }: { + { title, description, start_date, end_date, currency, day_count, reminder_days }: { title: string; description?: string; start_date?: string; end_date?: string; currency?: string; + day_count?: number; reminder_days?: number; }, ctx: McpContext, ) { @@ -111,7 +116,7 @@ export class TripsMcp { if (start_date && end_date && new Date(end_date) < new Date(start_date)) { return { content: [{ type: 'text' as const, text: 'End date must be after start date.' }], isError: true }; } - const { trip } = this.trips.create(ctx.userId, { title, description, start_date, end_date, currency }, MAX_MCP_TRIP_DAYS); + const { trip } = this.trips.create(ctx.userId, { title, description, start_date, end_date, currency, day_count, reminder_days }, MAX_MCP_TRIP_DAYS); return ok({ trip }); } @@ -121,12 +126,18 @@ export class TripsMcp { inputSchema: { tripId: z.number().int().positive(), title: z.string().min(1).max(200).optional(), - description: z.string().max(2000).optional(), + description: z.string().max(2000).nullable().optional().describe('Trip description; null removes it'), start_date: z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional(), end_date: z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional(), + clear_dates: z.boolean().optional().describe( + 'Drop both dates and turn the trip back into a dateless one. Cannot be combined with start_date or end_date; pair it with day_count to say how many days the dateless trip keeps'), currency: z.string().length(3).optional(), is_archived: z.boolean().optional().describe('Archive (true) or unarchive (false) the trip'), - cover_image: z.string().optional().describe('Cover image path, e.g. /uploads/covers/abc.jpg'), + cover_image: z.string().nullable().optional().describe('Cover image path, e.g. /uploads/covers/abc.jpg; null removes the cover'), + day_count: z.number().int().min(1).max(MAX_TRIP_DAYS).optional().describe( + 'Resize a trip without dates to this many days. Days that still hold content are never trimmed. Ignored while the trip has a date range, because the range decides the count'), + reminder_days: z.number().int().min(0).max(30).optional().describe( + 'How many days before departure the pre-trip reminder fires. 0 turns the reminder off'), date_shift_mode: z.enum(['keep_bookings', 'shift_all']).optional().describe( 'When changing dates: keep_bookings (default) keeps dated reservations/accommodations on their dates while day plans move; shift_all moves the whole itinerary, bookings included'), }, @@ -134,15 +145,18 @@ export class TripsMcp { access: { group: 'trips', mode: 'write' }, }) async updateTrip( - { tripId, title, description, start_date, end_date, currency, is_archived, cover_image, date_shift_mode }: { - tripId: number; title?: string; description?: string; start_date?: string; end_date?: string; - currency?: string; is_archived?: boolean; cover_image?: string; date_shift_mode?: 'keep_bookings' | 'shift_all'; + { tripId, title, description, start_date, end_date, clear_dates, currency, is_archived, cover_image, day_count, reminder_days, date_shift_mode }: { + tripId: number; title?: string; description?: string | null; start_date?: string; end_date?: string; + clear_dates?: boolean; currency?: string; is_archived?: boolean; cover_image?: string | null; + day_count?: number; reminder_days?: number; date_shift_mode?: 'keep_bookings' | 'shift_all'; }, ctx: McpContext, ) { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.trips.canAccessTrip(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('trip_edit', tripId, ctx.userId)) return permissionDenied(); + if (clear_dates && (start_date || end_date)) + return errorResult('clear_dates cannot be combined with start_date or end_date.'); if (start_date) { const d = new Date(start_date + 'T00:00:00Z'); if (Number.isNaN(d.getTime()) || d.toISOString().slice(0, 10) !== start_date) @@ -153,13 +167,40 @@ export class TripsMcp { if (Number.isNaN(d.getTime()) || d.toISOString().slice(0, 10) !== end_date) return { content: [{ type: 'text' as const, text: 'end_date is not a valid calendar date.' }], isError: true }; } + // updateTrip reads every field with a `!== undefined` presence sentinel, so a + // dated trip only goes back to dateless once an explicit null reaches it. A flag + // rather than a nullable start_date/end_date: clients that serialise an omitted + // optional as null would otherwise wipe the dates of every trip they touched. + const dates: { start_date?: string | null; end_date?: string | null } = clear_dates + ? { start_date: null, end_date: null } + : { start_date, end_date }; // update() re-anchors the budget before the trip row moves off the old // currency (#1543) and then runs the legacy updateTrip core. - const { updatedTrip } = await this.trips.update(tripId, ctx.userId, { title, description, start_date, end_date, currency, is_archived, cover_image, date_shift_mode }, 'user'); + const { updatedTrip } = await this.trips.update(tripId, ctx.userId, { title, description, ...dates, currency, is_archived, cover_image, day_count, reminder_days, date_shift_mode }, 'user'); this.guards.safeBroadcast(tripId, 'trip:updated', { trip: updatedTrip }); return ok({ trip: updatedTrip }); } + @Tool({ + name: 'search_cover_images', + description: 'Search Unsplash for candidate cover photos and return their URLs, thumbnails and photographer credits. Use this when the user wants a cover image but has no URL of their own, then pass the chosen photo\'s url back as update_trip\'s cover_image. Nothing is saved to a trip here.', + inputSchema: { + query: z.string().min(1).max(200).describe('What the cover should show, e.g. "Lisbon rooftops at sunset"'), + }, + annotations: TOOL_ANNOTATIONS_OPEN_WORLD_READONLY, + access: (ctx) => canReadTrips(ctx.scopes), + }) + async searchCoverImages({ query }: { query: string }, ctx: McpContext) { + try { + const result = await this.trips.searchCoverImages(query, ctx.userId); + if ('error' in result) return errorResult(result.error); + return ok({ photos: result.photos }); + } catch (err) { + console.error('Unsplash cover image error:', err); + return errorResult('Error searching for cover images'); + } + } + @Tool({ name: 'delete_trip', description: 'Delete a trip. Only the trip owner can delete it.', @@ -288,7 +329,7 @@ export class TripsMcp { @Tool({ name: 'add_trip_member', - description: 'Add a user to a trip by their username or email address. Only the trip owner can do this.', + description: 'Add a user to a trip by their username or email address. Needs the member_manage permission, which by default only the trip owner holds. Use create_trip_guest instead for a companion who has no TREK account.', inputSchema: { tripId: z.number().int().positive(), identifier: z.string().min(1).describe('Username or email of the user to add'), @@ -300,8 +341,11 @@ export class TripsMcp { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.trips.canAccessTrip(tripId, ctx.userId)) return noAccess(); const ownerRow = this.trips.getOwner(tripId); - if (!ownerRow || ownerRow.user_id !== ctx.userId) - return { content: [{ type: 'text' as const, text: 'Only the trip owner can add members.' }], isError: true }; + if (!ownerRow) return noAccess(); + // member_manage rather than a hardcoded owner test: the action is admin-lowerable + // in the permission matrix, and POST /api/trips/:id/members has always honoured + // that setting (and the admin bypass inside checkPermission) where this did not. + if (!this.guards.hasTripPermission('member_manage', tripId, ctx.userId)) return permissionDenied(); try { const result = this.members.addMember(tripId, identifier, ownerRow.user_id, ctx.userId); this.guards.safeBroadcast(tripId, 'member:added', { member: result.member }); @@ -314,7 +358,7 @@ export class TripsMcp { @Tool({ name: 'remove_trip_member', - description: 'Remove a member from a trip. Only the trip owner can do this.', + description: 'Remove somebody else from a trip. Needs the member_manage permission, which by default only the trip owner holds. When the user means themselves, prefer leave_trip: it says so plainly and needs no permission.', inputSchema: { tripId: z.number().int().positive(), memberId: z.number().int().positive().describe('User ID of the member to remove'), @@ -325,14 +369,38 @@ export class TripsMcp { async removeTripMember({ tripId, memberId }: { tripId: number; memberId: number }, ctx: McpContext) { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.trips.canAccessTrip(tripId, ctx.userId)) return noAccess(); - const ownerRow = this.trips.getOwner(tripId); - if (!ownerRow || ownerRow.user_id !== ctx.userId) - return { content: [{ type: 'text' as const, text: 'Only the trip owner can remove members.' }], isError: true }; + // Giving up your own access is not member management, so it carries no permission + // requirement: the same self-removal bypass DELETE /api/trips/:id/members/:userId has. + if (memberId !== ctx.userId && !this.guards.hasTripPermission('member_manage', tripId, ctx.userId)) + return permissionDenied(); this.members.removeMember(tripId, memberId); this.guards.safeBroadcast(tripId, 'member:removed', { userId: memberId }); return ok({ success: true }); } + @Tool({ + name: 'leave_trip', + description: 'Leave a trip you were invited to, giving up your own access to it. Prefer this over remove_trip_member whenever the user means themselves. The owner cannot leave their own trip.', + inputSchema: { + tripId: z.number().int().positive(), + }, + annotations: TOOL_ANNOTATIONS_DELETE, + access: { group: 'trips', mode: 'write' }, + }) + async leaveTrip({ tripId }: { tripId: number }, ctx: McpContext) { + if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); + if (!this.trips.canAccessTrip(tripId, ctx.userId)) return noAccess(); + const ownerRow = this.trips.getOwner(tripId); + if (!ownerRow) return noAccess(); + // removeMember only deletes a trip_members row, and the owner has none, so an + // owner calling this would get a success that changed nothing. Say so instead. + if (ownerRow.user_id === ctx.userId) + return errorResult('You own this trip, so you cannot leave it. Hand it to another member first, or delete it.'); + this.members.removeMember(tripId, ctx.userId); + this.guards.safeBroadcast(tripId, 'member:removed', { userId: ctx.userId }); + return ok({ success: true }); + } + // --- GUESTS (#1362) --- // // add_trip_member resolves an existing account by username or email, so a @@ -345,7 +413,7 @@ export class TripsMcp { @Tool({ name: 'create_trip_guest', - description: 'Add a travelling companion who has no TREK account to a trip. Use this when the person cannot be found by username or email — a guest can be assigned to budget splits, packing items, to-dos and day participants like any member, but never signs in and is never emailed. Only the trip owner can do this.', + description: 'Add a travelling companion who has no TREK account to a trip. Use this when the person cannot be found by username or email. A guest can be assigned to budget splits, packing items, to-dos and day participants like any member, but never signs in and is never emailed. Only the trip owner can do this.', inputSchema: { tripId: z.number().int().positive(), name: z.string().min(1).max(50).describe('Display name of the guest, e.g. "Anna"'), @@ -399,7 +467,7 @@ export class TripsMcp { @Tool({ name: 'delete_trip_guest', - description: 'Remove a guest from a trip. This deletes the guest outright — they exist only for this trip — and re-splits any expenses they were part of. Only the trip owner can do this.', + description: 'Remove a guest from a trip. This deletes the guest outright (they exist only for this trip) and re-splits any expenses they were part of. Only the trip owner can do this.', inputSchema: { tripId: z.number().int().positive(), guestId: z.number().int().positive().describe('User ID of the guest, from list_trip_members'), diff --git a/server/tests/unit/mcp/tools-trip-members.test.ts b/server/tests/unit/mcp/tools-trip-members.test.ts index 23ad93853..6057a4a30 100644 --- a/server/tests/unit/mcp/tools-trip-members.test.ts +++ b/server/tests/unit/mcp/tools-trip-members.test.ts @@ -38,7 +38,8 @@ vi.mock('../../../src/websocket', () => ({ broadcast: broadcastMock })); import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip, addTripMember } from '../../helpers/factories'; +import { invalidatePermissionsCache } from '../../../src/nest/permissions/permissions-cache'; +import { createUser, createAdmin, createTrip, addTripMember } from '../../helpers/factories'; import { createMcpHarness, parseToolResult, type McpHarness } from '../../helpers/mcp-harness'; beforeAll(() => { @@ -50,6 +51,9 @@ beforeEach(() => { resetTestDb(testDb); broadcastMock.mockClear(); delete process.env.DEMO_MODE; + // resetTestDb truncates app_settings, but the permission cache is module-scoped + // and would keep serving whatever the previous case configured. + invalidatePermissionsCache(); }); afterAll(() => { @@ -61,6 +65,12 @@ async function withHarness(userId: number, fn: (h: McpHarness) => Promise) try { await fn(h); } finally { await h.cleanup(); } } +/** Lower a configurable action the way the admin permission panel does. */ +function setPermission(action: string, level: string) { + testDb.prepare('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)').run(`perm_${action}`, level); + invalidatePermissionsCache(); +} + // --------------------------------------------------------------------------- // list_trip_members // --------------------------------------------------------------------------- @@ -161,6 +171,57 @@ describe('Tool: add_trip_member', () => { expect(result.isError).toBe(true); }); }); + + it('lets a collaborator add when member_manage sits at trip_member', async () => { + const { user: owner } = createUser(testDb); + const { user: collaborator } = createUser(testDb); + const { user: outsider } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, collaborator.id); + setPermission('member_manage', 'trip_member'); + await withHarness(collaborator.id, async (h) => { + const result = await h.client.callTool({ + name: 'add_trip_member', + arguments: { tripId: trip.id, identifier: outsider.username }, + }); + expect(result.isError).toBeFalsy(); + const row = testDb.prepare('SELECT invited_by FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, outsider.id) as any; + expect(row).toBeTruthy(); + expect(row.invited_by).toBe(collaborator.id); + }); + }); + + it('lets a site admin on the trip add a member without owning it', async () => { + const { user: owner } = createUser(testDb); + const { user: admin } = createAdmin(testDb); + const { user: outsider } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, admin.id); + await withHarness(admin.id, async (h) => { + const result = await h.client.callTool({ + name: 'add_trip_member', + arguments: { tripId: trip.id, identifier: outsider.username }, + }); + expect(result.isError).toBeFalsy(); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, outsider.id)).toBeTruthy(); + }); + }); + + it('still refuses a collaborator while member_manage sits at its trip_owner default', async () => { + const { user: owner } = createUser(testDb); + const { user: collaborator } = createUser(testDb); + const { user: outsider } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, collaborator.id); + await withHarness(collaborator.id, async (h) => { + const result = await h.client.callTool({ + name: 'add_trip_member', + arguments: { tripId: trip.id, identifier: outsider.username }, + }); + expect(result.isError).toBe(true); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, outsider.id)).toBeUndefined(); + }); + }); }); // --------------------------------------------------------------------------- @@ -209,6 +270,150 @@ describe('Tool: remove_trip_member', () => { expect(result.isError).toBe(true); }); }); + + it('leaves another member in place when a collaborator has no member_manage', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const { user: other } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + addTripMember(testDb, trip.id, other.id); + await withHarness(member.id, async (h) => { + const result = await h.client.callTool({ + name: 'remove_trip_member', + arguments: { tripId: trip.id, memberId: other.id }, + }); + expect(result.isError).toBe(true); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, other.id)).toBeTruthy(); + }); + }); + + it('lets a collaborator remove another member when member_manage sits at trip_member', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const { user: other } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + addTripMember(testDb, trip.id, other.id); + setPermission('member_manage', 'trip_member'); + await withHarness(member.id, async (h) => { + const result = await h.client.callTool({ + name: 'remove_trip_member', + arguments: { tripId: trip.id, memberId: other.id }, + }); + expect(result.isError).toBeFalsy(); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, other.id)).toBeUndefined(); + }); + }); + + it('lets a site admin on the trip remove a member without owning it', async () => { + const { user: owner } = createUser(testDb); + const { user: admin } = createAdmin(testDb); + const { user: other } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, admin.id); + addTripMember(testDb, trip.id, other.id); + await withHarness(admin.id, async (h) => { + const result = await h.client.callTool({ + name: 'remove_trip_member', + arguments: { tripId: trip.id, memberId: other.id }, + }); + expect(result.isError).toBeFalsy(); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, other.id)).toBeUndefined(); + }); + }); + + it('lets a member remove themselves without member_manage', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + await withHarness(member.id, async (h) => { + const result = await h.client.callTool({ + name: 'remove_trip_member', + arguments: { tripId: trip.id, memberId: member.id }, + }); + expect((parseToolResult(result) as any).success).toBe(true); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id)).toBeUndefined(); + }); + }); +}); + +// --------------------------------------------------------------------------- +// leave_trip +// --------------------------------------------------------------------------- + +describe('Tool: leave_trip', () => { + it('drops the caller from the roster', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + await withHarness(member.id, async (h) => { + const result = await h.client.callTool({ name: 'leave_trip', arguments: { tripId: trip.id } }); + expect((parseToolResult(result) as any).success).toBe(true); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id)).toBeUndefined(); + expect(testDb.prepare('SELECT id FROM trips WHERE id = ?').get(trip.id)).toBeTruthy(); + }); + }); + + it('broadcasts member:removed for the leaving user', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + await withHarness(member.id, async (h) => { + await h.client.callTool({ name: 'leave_trip', arguments: { tripId: trip.id } }); + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'member:removed', expect.objectContaining({ userId: member.id })); + }); + }); + + it('leaves the other members alone', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const { user: other } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + addTripMember(testDb, trip.id, other.id); + await withHarness(member.id, async (h) => { + await h.client.callTool({ name: 'leave_trip', arguments: { tripId: trip.id } }); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, other.id)).toBeTruthy(); + }); + }); + + it('refuses the owner and keeps the trip theirs', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await withHarness(owner.id, async (h) => { + const result = await h.client.callTool({ name: 'leave_trip', arguments: { tripId: trip.id } }); + expect(result.isError).toBe(true); + const row = testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as any; + expect(row.user_id).toBe(owner.id); + }); + }); + + it('returns access denied for a non-member', async () => { + const { user: outsider } = createUser(testDb); + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await withHarness(outsider.id, async (h) => { + const result = await h.client.callTool({ name: 'leave_trip', arguments: { tripId: trip.id } }); + expect(result.isError).toBe(true); + }); + }); + + it('blocks demo user', async () => { + process.env.DEMO_MODE = 'true'; + const { user: owner } = createUser(testDb); + const { user } = createUser(testDb, { email: 'demo@nomad.app' }); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, user.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'leave_trip', arguments: { tripId: trip.id } }); + expect(result.isError).toBe(true); + expect(testDb.prepare('SELECT user_id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, user.id)).toBeTruthy(); + }); + }); }); // --------------------------------------------------------------------------- @@ -411,7 +616,7 @@ describe('Tool: create_trip_guest', () => { expect(row.is_guest).toBe(1); expect(row.password_hash).toBe(''); expect(row.display_name).toBe('Anna'); - // A synthetic address on an undeliverable domain — a guest is never emailed. + // A synthetic address on an undeliverable domain: a guest is never emailed. expect(row.email).toMatch(/@guests\.invalid$/); }); }); @@ -654,3 +859,38 @@ describe('Tool: delete_trip_guest', () => { }); }); }); + +// --------------------------------------------------------------------------- +// The guest routes carry TripOwnerGuard, which deliberately does NOT consult the +// permission matrix: handing a trip over and creating or deleting guests are the +// things a collaborator must never do, however generously the trip is configured. +// Lowering member_manage moves add/remove_trip_member and must move nothing here. +// --------------------------------------------------------------------------- + +describe('Guest tools stay owner-only', () => { + it('refuses a collaborator all three guest tools even with member_manage at trip_member', async () => { + const { user: owner } = createUser(testDb); + const { user: collaborator } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, collaborator.id); + let guestId = 0; + await withHarness(owner.id, async (h) => { guestId = await makeGuest(h, trip.id, 'Anna'); }); + + setPermission('member_manage', 'trip_member'); + await withHarness(collaborator.id, async (h) => { + expect((await h.client.callTool({ + name: 'create_trip_guest', arguments: { tripId: trip.id, name: 'Bea' }, + })).isError).toBe(true); + expect((await h.client.callTool({ + name: 'rename_trip_guest', arguments: { tripId: trip.id, guestId, name: 'Renamed' }, + })).isError).toBe(true); + expect((await h.client.callTool({ + name: 'delete_trip_guest', arguments: { tripId: trip.id, guestId }, + })).isError).toBe(true); + }); + + const row = testDb.prepare('SELECT display_name FROM users WHERE id = ?').get(guestId) as any; + expect(row.display_name).toBe('Anna'); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM users WHERE is_guest = 1').get()).toEqual({ n: 1 }); + }); +}); diff --git a/server/tests/unit/mcp/tools-trips.test.ts b/server/tests/unit/mcp/tools-trips.test.ts index e47288a90..a87550953 100644 --- a/server/tests/unit/mcp/tools-trips.test.ts +++ b/server/tests/unit/mcp/tools-trips.test.ts @@ -7,7 +7,7 @@ * notice riding the attach ctx, and the scope gating (declarative * trips:write markers + the canReadTrips/canDeleteTrips predicates). */ -import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from 'vitest'; const { testDb, dbMock } = vi.hoisted(() => { const Database = require('better-sqlite3'); @@ -55,6 +55,11 @@ beforeEach(() => { delete process.env.DEMO_MODE; }); +// Only search_cover_images stubs fetch, and it stubs a different response per case. +afterEach(() => { + vi.unstubAllGlobals(); +}); + afterAll(() => { testDb.close(); }); @@ -134,6 +139,57 @@ describe('Tool: create_trip', () => { expect(result.isError).toBe(true); }); }); + + it('gives a dateless trip the requested day_count instead of the default 7', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'create_trip', arguments: { title: 'Open Ended', day_count: 12 } }); + const data = parseToolResult(result) as any; + const days = testDb.prepare('SELECT COUNT(*) as c FROM days WHERE trip_id = ?').get(data.trip.id) as { c: number }; + expect(days.c).toBe(12); + const row = testDb.prepare('SELECT start_date, end_date FROM trips WHERE id = ?').get(data.trip.id) as any; + expect(row.start_date).toBeNull(); + expect(row.end_date).toBeNull(); + }); + }); + + it('refuses a day_count outside 1..365', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + expect((await h.client.callTool({ name: 'create_trip', arguments: { title: 'Zero', day_count: 0 } })).isError).toBe(true); + expect((await h.client.callTool({ name: 'create_trip', arguments: { title: 'Huge', day_count: 400 } })).isError).toBe(true); + expect(testDb.prepare('SELECT COUNT(*) as c FROM trips').get()).toEqual({ c: 0 }); + }); + }); + + it('stores the requested reminder_days', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'create_trip', arguments: { title: 'Reminded', reminder_days: 14 } }); + const data = parseToolResult(result) as any; + const row = testDb.prepare('SELECT reminder_days FROM trips WHERE id = ?').get(data.trip.id) as any; + expect(row.reminder_days).toBe(14); + }); + }); + + it('turns the reminder off with reminder_days 0 rather than falling back to 3', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'create_trip', arguments: { title: 'Quiet', reminder_days: 0 } }); + const data = parseToolResult(result) as any; + const row = testDb.prepare('SELECT reminder_days FROM trips WHERE id = ?').get(data.trip.id) as any; + expect(row.reminder_days).toBe(0); + }); + }); + + it('refuses a reminder_days outside 0..30', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + expect((await h.client.callTool({ name: 'create_trip', arguments: { title: 'Early', reminder_days: 31 } })).isError).toBe(true); + expect((await h.client.callTool({ name: 'create_trip', arguments: { title: 'Negative', reminder_days: -1 } })).isError).toBe(true); + expect(testDb.prepare('SELECT COUNT(*) as c FROM trips').get()).toEqual({ c: 0 }); + }); + }); }); // --------------------------------------------------------------------------- @@ -272,6 +328,173 @@ describe('Tool: update_trip', () => { ).all(ownPlanId, user.id) as { date: string }[]; expect(shifted.map(r => r.date)).toEqual(['2026-09-09', '2026-09-10']); }); + + it('clear_dates turns a dated trip back into a dateless one and un-dates its days', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-07-01', end_date: '2026-07-05' }); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, clear_dates: true } }); + expect(result.isError).toBeFalsy(); + const row = testDb.prepare('SELECT start_date, end_date FROM trips WHERE id = ?').get(trip.id) as any; + expect(row.start_date).toBeNull(); + expect(row.end_date).toBeNull(); + const dated = testDb.prepare('SELECT COUNT(*) as c FROM days WHERE trip_id = ? AND date IS NOT NULL').get(trip.id) as { c: number }; + expect(dated.c).toBe(0); + }); + }); + + it('clear_dates with a day_count resizes the now dateless trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-07-01', end_date: '2026-07-05' }); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, clear_dates: true, day_count: 3 } }); + const days = testDb.prepare('SELECT COUNT(*) as c FROM days WHERE trip_id = ?').get(trip.id) as { c: number }; + expect(days.c).toBe(3); + }); + }); + + it('refuses clear_dates alongside a start_date and leaves the trip untouched', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-07-01', end_date: '2026-07-05' }); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'update_trip', + arguments: { tripId: trip.id, clear_dates: true, start_date: '2026-08-01' }, + }); + expect(result.isError).toBe(true); + const row = testDb.prepare('SELECT start_date FROM trips WHERE id = ?').get(trip.id) as any; + expect(row.start_date).toBe('2026-07-01'); + }); + }); + + it('resizes a dateless trip with day_count', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + testDb.prepare('INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, NULL), (?, 2, NULL)').run(trip.id, trip.id); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, day_count: 6 } }); + const days = testDb.prepare('SELECT COUNT(*) as c FROM days WHERE trip_id = ?').get(trip.id) as { c: number }; + expect(days.c).toBe(6); + }); + }); + + it('refuses a day_count outside 1..365', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Untouched' }); + await withHarness(user.id, async (h) => { + expect((await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, day_count: 0 } })).isError).toBe(true); + expect((await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, day_count: 400 } })).isError).toBe(true); + expect(testDb.prepare('SELECT COUNT(*) as c FROM days WHERE trip_id = ?').get(trip.id)).toEqual({ c: 0 }); + }); + }); + + it('changes reminder_days and can turn the reminder off', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, reminder_days: 10 } }); + expect((testDb.prepare('SELECT reminder_days FROM trips WHERE id = ?').get(trip.id) as any).reminder_days).toBe(10); + await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, reminder_days: 0 } }); + expect((testDb.prepare('SELECT reminder_days FROM trips WHERE id = ?').get(trip.id) as any).reminder_days).toBe(0); + }); + }); + + it('refuses a reminder_days outside 0..30 and keeps the stored value', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, reminder_days: 31 } }); + expect(result.isError).toBe(true); + expect((testDb.prepare('SELECT reminder_days FROM trips WHERE id = ?').get(trip.id) as any).reminder_days).toBe(3); + }); + }); + + it('clears the description with an explicit null', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { description: 'A great trip' }); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, description: null } }); + const row = testDb.prepare('SELECT description FROM trips WHERE id = ?').get(trip.id) as any; + expect(row.description).toBeNull(); + }); + }); + + it('clears the cover image with an explicit null', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + testDb.prepare('UPDATE trips SET cover_image = ? WHERE id = ?').run('/uploads/covers/old.jpg', trip.id); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, cover_image: null } }); + const row = testDb.prepare('SELECT cover_image FROM trips WHERE id = ?').get(trip.id) as any; + expect(row.cover_image).toBeNull(); + }); + }); +}); + +// --------------------------------------------------------------------------- +// search_cover_images +// --------------------------------------------------------------------------- + +describe('Tool: search_cover_images', () => { + function stubUnsplash(body: unknown, init: { ok?: boolean; status?: number } = {}) { + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: init.ok ?? true, + status: init.status ?? 200, + json: async () => body, + }))); + } + + it('returns the photo candidates for a query', async () => { + const { user } = createUser(testDb); + stubUnsplash({ + results: [{ + id: 'p1', + urls: { regular: 'https://images.unsplash.com/photo-1.jpg', small: 'https://images.unsplash.com/thumb-1.jpg' }, + alt_description: 'Rooftops at sunset', + user: { name: 'Ada L.' }, + links: { html: 'https://unsplash.com/photos/p1' }, + }], + }); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'search_cover_images', arguments: { query: 'Lisbon rooftops' } }); + const data = parseToolResult(result) as any; + expect(data.photos).toHaveLength(1); + expect(data.photos[0].url).toBe('https://images.unsplash.com/photo-1.jpg'); + expect(data.photos[0].photographer).toBe('Ada L.'); + }); + }); + + it('reports an upstream failure as a tool error', async () => { + const { user } = createUser(testDb); + stubUnsplash({ errors: ['Rate Limit Exceeded'] }, { ok: false, status: 429 }); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'search_cover_images', arguments: { query: 'Lisbon' } }); + expect(result.isError).toBe(true); + expect((result.content as { text: string }[])[0].text).toBe('Rate Limit Exceeded'); + }); + }); + + it('refuses an empty query without reaching Unsplash', async () => { + const { user } = createUser(testDb); + const fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'search_cover_images', arguments: { query: '' } }); + expect(result.isError).toBe(true); + expect(fetchMock).not.toHaveBeenCalled(); + }); + }); + + it('saves nothing on the trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + stubUnsplash({ results: [] }); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'search_cover_images', arguments: { query: 'Lisbon' } }); + const row = testDb.prepare('SELECT cover_image FROM trips WHERE id = ?').get(trip.id) as any; + expect(row.cover_image).toBeNull(); + }); + }); }); // --------------------------------------------------------------------------- @@ -674,8 +897,8 @@ describe('scope gating', () => { } } - const WRITE_TOOLS = ['create_trip', 'update_trip', 'add_trip_member', 'remove_trip_member', 'copy_trip']; - const READ_TOOLS = ['list_trip_members', 'export_trip_ics']; + const WRITE_TOOLS = ['create_trip', 'update_trip', 'add_trip_member', 'remove_trip_member', 'leave_trip', 'copy_trip']; + const READ_TOOLS = ['list_trip_members', 'export_trip_ics', 'search_cover_images']; const NAV_TOOLS = ['list_trips', 'get_trip_summary']; const SHARE_TOOLS = ['get_share_link', 'create_share_link', 'delete_share_link'];