diff --git a/server/src/nest/plugins/contributions/plugin-contributions.module.ts b/server/src/nest/plugins/contributions/plugin-contributions.module.ts index f080fe28a..95840bc7a 100644 --- a/server/src/nest/plugins/contributions/plugin-contributions.module.ts +++ b/server/src/nest/plugins/contributions/plugin-contributions.module.ts @@ -4,6 +4,7 @@ import { AddonsModule } from '../../addons/addons.module'; import { JourneyDomainModule } from '../../journey/journey-domain.module'; import { PlaceDetailsController } from './place-details.controller'; import { TripWarningsController } from './trip-warnings.controller'; +import { TripWarningsMcp } from './trip-warnings.mcp'; import { ViewContributionsController } from './view-contributions.controller'; import { TripCardContributionsController } from './trip-card-contributions.controller'; import { PluginPhotosController } from './plugin-photos.controller'; @@ -46,5 +47,9 @@ import { JournalEntryRowsController } from './journal-entry-rows.controller'; AtlasLayersController, JournalEntryRowsController, ], + // The only contribution with an MCP counterpart so far. It belongs to this module + // rather than to the trip read model because the plugin runtime and the trip + // aggregate already import each other's modules; see trip-warnings.mcp.ts. + providers: [TripWarningsMcp], }) export class PluginContributionsModule {} diff --git a/server/src/nest/plugins/contributions/trip-warnings.mcp.ts b/server/src/nest/plugins/contributions/trip-warnings.mcp.ts new file mode 100644 index 000000000..6316eb1b8 --- /dev/null +++ b/server/src/nest/plugins/contributions/trip-warnings.mcp.ts @@ -0,0 +1,87 @@ +import { z } from 'zod'; +import { McpController, Tool, TOOL_ANNOTATIONS_READONLY, ok, type McpContext, type McpTextResult } from '../../../nest-mcp'; +import { noAccess } from '../../../mcp/tools/_shared'; +import { DatabaseService } from '../../database/database.service'; +import { pluginsEnabled } from '../kill-switch'; +import { PluginHooks } from '../plugin-hooks.service'; +import { stripEmoji } from '../text-sanitize'; + +/** + * The MCP half of GET /api/trip-warnings/:tripId (#1429). + * + * The web UI shows plugin warnings as a banner on the trip it belongs to, so a user + * reviewing a trip sees "this hotel booking has no check-out day" without asking for + * it. An assistant reviewing the same trip saw nothing: get_trip_summary loads the + * trip's own rows and a warning is not a row, it is a plugin's live verdict on them. + * + * It is a tool of its own rather than a section of get_trip_summary because the two + * have incompatible costs and dependencies. The summary is a synchronous SQLite read + * out of TripReadModelService; a warning is an IPC round trip into every installed + * warning provider, each with a 5 s budget. Folding it in would make the documented + * "call this once to load the trip" loader wait on third-party child processes, and + * would need TripReadModelModule to import the plugin runtime, which imports + * TripsModule, which imports TripReadModelModule. + */ +type Level = 'info' | 'warning' | 'error'; +interface Warning { + pluginId: string; + level: Level; + message: string; + dayId?: number; + placeId?: number; +} + +// Same two caps the REST route applies, for the same reason: one provider must not be +// able to flood the answer, and a warning is a banner line rather than a document. +const MAX_WARNINGS = 20; +const MESSAGE_MAX = 300; + +@McpController() +export class TripWarningsMcp { + constructor( + private readonly hooks: PluginHooks, + private readonly dbs: DatabaseService, + ) {} + + @Tool({ + name: 'get_trip_warnings', + description: 'Problems installed plugins report about a trip: a plugin flagging that something is wrong with the itinerary, such as an accommodation with no check-out, a day that cannot be travelled in the time it allows, or a place closed on the day it is planned for. get_trip_summary returns the trip\'s stored data and never these verdicts, so call this as well before reviewing a trip, reporting on it, or telling the user it looks fine. Returns an empty list when no installed plugin contributes warnings, which is the normal case.', + inputSchema: { + tripId: z.number().int().positive(), + }, + annotations: TOOL_ANNOTATIONS_READONLY, + access: { group: 'trips', mode: 'read' }, + }) + async getTripWarnings({ tripId }: { tripId: number }, ctx: McpContext): Promise { + // Access first, so the answer to "may I look at this trip" does not depend on + // whether an admin has the plugin system switched on. + if (!this.dbs.canAccessTrip(tripId, ctx.userId)) return noAccess(); + if (!pluginsEnabled()) return ok({ warnings: [] }); + + const ids = this.hooks.providersOf('warningProvider'); + const perProvider = await Promise.all( + ids.map(async (id): Promise => { + try { + const raw = (await this.hooks.tripWarnings(id, tripId, ctx.userId)) as unknown; + const list = Array.isArray(raw) ? (raw as unknown[]) : []; + // Drop non-object elements BEFORE the cap: one null in the array would throw + // inside map() and the catch below would discard everything this provider + // returned, and the cap should count valid entries only. + return list + .filter((w): w is Record => !!w && typeof w === 'object') + .slice(0, MAX_WARNINGS) + .map((w) => ({ + pluginId: id, + level: w.level === 'error' || w.level === 'info' ? (w.level as Level) : 'warning', + message: stripEmoji(String(w.message ?? '')).slice(0, MESSAGE_MAX), + dayId: typeof w.dayId === 'number' ? w.dayId : undefined, + placeId: typeof w.placeId === 'number' ? w.placeId : undefined, + })); + } catch { + return []; // a slow or failing provider contributes nothing, it does not fail the tool + } + }), + ); + return ok({ warnings: perProvider.flat().filter((w) => w.message) }); + } +} diff --git a/server/tests/unit/mcp/tools-trip-warnings.test.ts b/server/tests/unit/mcp/tools-trip-warnings.test.ts new file mode 100644 index 000000000..81439b980 --- /dev/null +++ b/server/tests/unit/mcp/tools-trip-warnings.test.ts @@ -0,0 +1,330 @@ +/** + * Unit tests for the get_trip_warnings MCP tool (src/nest/plugins/contributions/ + * trip-warnings.mcp.ts), the MCP counterpart of GET /api/trip-warnings/:tripId. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA foreign_keys = ON'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + getPlaceWithTags: () => null, + canAccessTrip: (tripId: number, userId: number) => + db + .prepare( + 'SELECT t.id FROM trips t LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL)', + ) + .get(userId, tripId, userId), + isOwner: (tripId: number, userId: number) => + !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId), + }; + return { testDb: db, dbMock: mock }; +}); + +const { broadcastMock, pluginsEnabled } = vi.hoisted(() => ({ + broadcastMock: vi.fn(), + pluginsEnabled: vi.fn(() => true), +})); + +vi.mock('../../../src/db/database', () => dbMock); +vi.mock('../../../src/websocket', () => ({ broadcast: broadcastMock })); +vi.mock('../../../src/config', () => ({ + JWT_SECRET: 'test-jwt-secret-for-trek-testing-only', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, +})); +// The admin kill switch reads live env; drive it from the test instead of the process. +vi.mock('../../../src/nest/plugins/kill-switch', () => ({ pluginsEnabled })); + +import { runMigrations } from '../../../src/db/migrations'; +import { createTables } from '../../../src/db/schema'; +import { addTripMember, createTrip, createUser } from '../../helpers/factories'; +import { createMcpHarness, parseToolResult, type McpHarness } from '../../helpers/mcp-harness'; +import { resetTestDb } from '../../helpers/test-db'; +import { PluginHooks } from '../../../src/nest/plugins/plugin-hooks.service'; + +// The harness builds the registry itself, so the fan-out is played on the prototype +// (the tools-transit.test.ts pattern). vitest is not configured to auto-restore, so +// every case restates what it wants in beforeEach. +const providersOfMock = vi.spyOn(PluginHooks.prototype, 'providersOf'); +const tripWarningsMock = vi.spyOn(PluginHooks.prototype, 'tripWarnings'); + +beforeAll(() => { + createTables(testDb); + runMigrations(testDb); +}); + +beforeEach(() => { + resetTestDb(testDb); + broadcastMock.mockClear(); + pluginsEnabled.mockReturnValue(true); + providersOfMock.mockReset().mockReturnValue([]); + tripWarningsMock.mockReset().mockResolvedValue([]); +}); + +afterAll(() => { + testDb.close(); +}); + +async function withHarness( + userId: number, + fn: (h: McpHarness) => Promise, + scopes?: string[] | null, +) { + const h = await createMcpHarness({ userId, withResources: false, scopes: scopes ?? null }); + try { await fn(h); } finally { await h.cleanup(); } +} + +interface WarningsPayload { + warnings: { pluginId: string; level: string; message: string; dayId?: number; placeId?: number }[]; +} + +const call = async (h: McpHarness, tripId: number) => + h.client.callTool({ name: 'get_trip_warnings', arguments: { tripId } }); + +// --------------------------------------------------------------------------- +// Happy path +// --------------------------------------------------------------------------- + +describe('Tool: get_trip_warnings', () => { + it('merges every provider, tags each warning with the plugin that raised it', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Kyoto' }); + providersOfMock.mockReturnValue(['seasons', 'logistics']); + tripWarningsMock.mockImplementation(async (pluginId: string) => + pluginId === 'seasons' + ? [{ level: 'info', message: 'Rainy week ahead' }] + : [{ level: 'error', message: 'Day 3 has no way back', dayId: 31 }], + ); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload.warnings).toEqual([ + { pluginId: 'seasons', level: 'info', message: 'Rainy week ahead' }, + { pluginId: 'logistics', level: 'error', message: 'Day 3 has no way back', dayId: 31 }, + ]); + expect(providersOfMock).toHaveBeenCalledWith('warningProvider'); + expect(tripWarningsMock).toHaveBeenCalledWith('seasons', trip.id, user.id); + }); + }); + + it('carries the place link through and defaults an unknown level to warning', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['opening-hours']); + tripWarningsMock.mockResolvedValue([{ level: 'catastrophe', message: 'Museum closed Mondays', placeId: 7 }]); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload.warnings).toEqual([ + { pluginId: 'opening-hours', level: 'warning', message: 'Museum closed Mondays', placeId: 7 }, + ]); + }); + }); + + it('answers with an empty list when no plugin provides warnings', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload).toEqual({ warnings: [] }); + expect(tripWarningsMock).not.toHaveBeenCalled(); + }); + }); + + it('is readable by a collaborator, not just the owner', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + providersOfMock.mockReturnValue(['logistics']); + tripWarningsMock.mockResolvedValue([{ level: 'warning', message: 'Tight connection' }]); + + await withHarness(member.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload.warnings).toHaveLength(1); + expect(tripWarningsMock).toHaveBeenCalledWith('logistics', trip.id, member.id); + }); + }); +}); + +// --------------------------------------------------------------------------- +// Access +// --------------------------------------------------------------------------- + +describe('get_trip_warnings access', () => { + it('refuses a trip the caller is not on, without asking any provider', async () => { + const { user: owner } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + providersOfMock.mockReturnValue(['logistics']); + + await withHarness(stranger.id, async (h) => { + const result = await call(h, trip.id); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toContain('Trip not found or access denied.'); + expect(tripWarningsMock).not.toHaveBeenCalled(); + }); + }); + + it('refuses a trip that does not exist', async () => { + const { user } = createUser(testDb); + + await withHarness(user.id, async (h) => { + const result = await call(h, 999999); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toContain('Trip not found or access denied.'); + }); + }); + + it('checks trip access before the kill switch, so a stranger never learns the switch is off', async () => { + const { user: owner } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + pluginsEnabled.mockReturnValue(false); + + await withHarness(stranger.id, async (h) => { + const result = await call(h, trip.id); + expect(result.isError).toBe(true); + }); + }); + + it('rides trips:read: present with it, absent without it', async () => { + const { user } = createUser(testDb); + + await withHarness(user.id, async (h) => { + const names = (await h.client.listTools()).tools.map(t => t.name); + expect(names).toContain('get_trip_warnings'); + }, ['trips:read']); + + await withHarness(user.id, async (h) => { + const names = (await h.client.listTools()).tools.map(t => t.name); + expect(names).not.toContain('get_trip_warnings'); + }, ['places:read']); + }); +}); + +// --------------------------------------------------------------------------- +// Degrading rather than failing +// --------------------------------------------------------------------------- + +describe('get_trip_warnings resilience', () => { + it('answers empty when an admin has the plugin system switched off', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + pluginsEnabled.mockReturnValue(false); + providersOfMock.mockReturnValue(['logistics']); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload).toEqual({ warnings: [] }); + expect(providersOfMock).not.toHaveBeenCalled(); + }); + }); + + it('drops a provider that throws or times out and keeps the rest', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['broken', 'healthy']); + tripWarningsMock.mockImplementation(async (pluginId: string) => { + if (pluginId === 'broken') throw new Error('hook timed out after 5000ms'); + return [{ level: 'warning', message: 'Still here' }]; + }); + + await withHarness(user.id, async (h) => { + const result = await call(h, trip.id); + expect(result.isError).toBeFalsy(); + const payload = parseToolResult(result) as WarningsPayload; + expect(payload.warnings).toEqual([{ pluginId: 'healthy', level: 'warning', message: 'Still here' }]); + }); + }); + + it('ignores a provider that answers with something other than an array', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['confused']); + tripWarningsMock.mockResolvedValue({ warnings: 'lots' }); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload).toEqual({ warnings: [] }); + }); + }); + + it('skips null entries instead of losing the whole provider to them', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['sloppy']); + tripWarningsMock.mockResolvedValue([null, { level: 'info', message: 'Survives' }, 'nope']); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload.warnings).toEqual([{ pluginId: 'sloppy', level: 'info', message: 'Survives' }]); + }); + }); + + it('drops a warning with no message rather than emitting a blank line', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['sloppy']); + tripWarningsMock.mockResolvedValue([{ level: 'error' }, { level: 'error', message: '' }]); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload).toEqual({ warnings: [] }); + }); + }); +}); + +// --------------------------------------------------------------------------- +// Caps +// --------------------------------------------------------------------------- + +describe('get_trip_warnings caps', () => { + it('caps a flooding provider at 20 warnings and truncates an oversized message', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['flood']); + tripWarningsMock.mockResolvedValue([ + { level: 'warning', message: 'z'.repeat(1000) }, + ...Array.from({ length: 50 }, (_v, i) => ({ level: 'info', message: `w${i}` })), + ]); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload.warnings).toHaveLength(20); + expect(payload.warnings[0].message).toHaveLength(300); + }); + }); + + it('caps each provider separately, so two providers can contribute 40', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['a', 'b']); + tripWarningsMock.mockResolvedValue( + Array.from({ length: 50 }, (_v, i) => ({ level: 'info', message: `w${i}` })), + ); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload.warnings).toHaveLength(40); + }); + }); + + it('strips emojis, because the assistant relays this text as TREK chrome', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + providersOfMock.mockReturnValue(['loud']); + tripWarningsMock.mockResolvedValue([{ level: 'error', message: '🔥 Overbooked!' }]); + + await withHarness(user.id, async (h) => { + const payload = parseToolResult(await call(h, trip.id)) as WarningsPayload; + expect(payload.warnings[0].message).toBe('Overbooked!'); + }); + }); +});