diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 2025e4a92..4ab6a207f 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -71,7 +71,7 @@ Nest owns everything. Every domain is a DI module under `src/nest//` (`c - **`src/bootstrap.ts` — `buildApp()`** is the single builder for production and the test harness. Everything on the raw Express instance (global middleware, uploads, discovery metadata, statics, the named `jsonParser`/`urlencodedParser` wrappers) is registered **before `app.init()`**, because Nest throws `NotFoundException` on unmatched routes and nothing after init is reachable. The wrappers exempt `/mcp` so the MCP SDK reads the raw stream — **never `@Body()` a `/mcp` route**. Don't add routes to that pre-init shell; it bypasses every global guard, interceptor and filter. - **Realtime**: `/ws` is a Nest gateway in `src/nest/realtime/`. Inject `RealtimeService`; tests hand in a `FakeRealtimeService` (`tests/helpers/fake-realtime.ts`) or spy on the container's instance instead of mocking the transport module. Trip mutations forward `X-Socket-Id` so the originating client doesn't echo its own change. -- **MCP**: `src/nest-mcp/` is the decorator/registry layer (`@McpController`/`@Tool`/`@Resource`/`@Prompt`; see its `README.md`); tools live in `.mcp.ts` files beside each module; the HTTP transport is `src/nest/mcp-transport/`; `src/mcp/` holds process-wide state only (scopes, session manager, boot policy). Every tool invocation writes an audit row through the registry's `onInvoke` seam. The demo-mode write block is the registry's `toolGate` (`trekDemoToolGate`): every tool not annotated read-only refuses a demo account there, so a tool never checks demo mode itself (ESLint refuses it in `*.mcp.ts`). MCP tools are adapters over the same services and permission checks as REST — a change on one side lands on the other in the same change. +- **MCP**: `src/nest-mcp/` is the decorator/registry layer (`@McpController`/`@Tool`/`@Resource`/`@Prompt`; see its `README.md`); tools live in `.mcp.ts` files beside each module; the HTTP transport is `src/nest/mcp-transport/`; `src/mcp/` holds process-wide state only (scopes, session manager, boot policy). Every tool invocation writes an audit row through the registry's `onInvoke` seam. The demo-mode write block is the registry's `toolGate` (`trekDemoToolGate`): every tool not annotated read-only refuses a demo account there (except the write tools in `DEMO_UNGATED_WRITE_TOOLS`, which never had a demo check), so a tool never checks demo mode itself (ESLint refuses it in `*.mcp.ts`). MCP tools are adapters over the same services and permission checks as REST — a change on one side lands on the other in the same change. - **Scheduling**: `src/nest/scheduling/` (`CronRegistrarService`) is the one way to schedule a cron. Jobs are `*.job.ts` providers in their owning domain. The registrar refuses to schedule under `NODE_ENV=test` (an integration test pins it). **Never use a `@Cron` decorator**: it bypasses that gate. Every tick first takes the job's row in `scheduler_leases` (`SchedulerLeasesRepository.acquire`, one conditional upsert that decides the race), renews it while it runs and keeps it for a short settle window afterwards, so two processes on one database never run the same tick; a job needs nothing of its own for this. Claims on individual rows (the reminder jobs' `claimReminder`) stay where they are: the lease decides who runs a tick, the claim what a tick may send. - **Plugins** (`src/nest/plugins/`): the child-process runtime (`supervisor/`, `runtime/plugin-host-entry.ts`, `protocol/envelope.ts`) is **intentionally not Nest**; it is the security boundary. `host/rpc-host.ts` registers an RPC method at spawn only if the plugin holds the unlocking permission (registration = authorization). Install-time gates live in `install/`; the manifest's `trek` version range fails closed, and `TREK_PLUGINS_IGNORE_TREK_RANGE` downgrades that to a warning. The protocol tables in `protocol/envelope.ts` (+ `src/plugin-event-sink.ts`) are **generated** into `plugin-sdk/` and `shared/`: run `gen:plugin-facts` after touching them and commit the output. What a plugin gets back is `protocol/output-contract.ts`: every wire method has an entry, and an entity result (`trips.getById`, `todos.list`, `vacay.mine`, ...) is cut down to its published fields in `rpc-host.ts` on the way out, together with every child row it carries whose field is listed in `PLUGIN_ENTITY_NESTED` (a day's `notes_items`, a reservation's `endpoints`). A new column on any whole-row table behind those entities, child tables included, fails `tests/unit/plugins/output-contract.schema.test.ts` until it is listed as published or withheld (the SDK's `PLUGIN_ENTITY_FIELDS` and `PLUGIN_ENTITY_NESTED` are generated from it); a method left as `readModel` must return a value built field by field from named columns. A plugin's own SQL (`host/plugin-data.service.ts`) is held to a row cap, and `query`, `tx` and an `exec` script to a time budget checked between rows and statements (`migrate` is deliberately not timed). `runtime/egress-policy.ts` is a hand-kept mirror guarded by a parity test in `plugin-sdk/`; change both sides together. - **Storage**: `src/nest/storage/` is the upload driver abstraction (local / S3 / mirror behind one interface, with an admin surface and a usage job). Domain services never touch the filesystem or the S3 SDK directly. Where the data lives (`data/`, `uploads/`, backups, logs, key files, the DB file) is resolved in one place, `src/app-config/data-paths.ts`; a provider that reads one injects `DataPathsService`, plain helper modules shared with code outside the container call `resolveDataPaths()` (the list is in `src/app-config/README.md`), and nothing anchors a data path on its own `__dirname` or on `process.cwd()`. diff --git a/server/src/mcp/nest-mcp-policy.ts b/server/src/mcp/nest-mcp-policy.ts index 7dad73ae9..baf27084c 100644 --- a/server/src/mcp/nest-mcp-policy.ts +++ b/server/src/mcp/nest-mcp-policy.ts @@ -57,16 +57,33 @@ const VALID_GROUP_MODES: ReadonlySet = new Set(ALL_SCOPES); export const trekMcpValidateAccess: McpAccessValidator = ({ group, mode }) => VALID_GROUP_MODES.has(`${group}:${mode}`) ? null : `no '${group}:${mode}' scope in SCOPES`; +/** + * Write tools a demo account has always been able to call: none of them ever + * carried a demo check of its own, so the central gate lets them through to + * keep that. Taking one off this list blocks it for demo accounts, which is a + * behaviour change of its own. tests/unit/mcp/demo-tool-gate.test.ts pins the + * list. + */ +export const DEMO_UNGATED_WRITE_TOOLS: ReadonlySet = new Set([ + 'sync_trip_documents', + 'create_manual_school_holiday_country', + 'create_manual_school_holiday_region', + 'update_manual_school_holiday_region', + 'delete_manual_school_holiday_region', + 'delete_manual_school_holiday_country', +]); + /** * True for a tool the demo gate holds back: every tool that does not declare - * itself read-only. The annotation, not `access.mode`, is the signal, because - * the share/content scopes cover reads too (get_share_link, - * get_trip_calendar_feed, read_trip_file) and the predicate-gated tools carry - * no mode at all. tests/unit/mcp/demo-tool-gate.test.ts holds the two in step: - * every `mode: 'read'` tool is read-only and every `mode: 'write'` one is not. + * itself read-only, except the ones in DEMO_UNGATED_WRITE_TOOLS. The + * annotation, not `access.mode`, is the signal, because the share/content + * scopes cover reads too (get_share_link, get_trip_calendar_feed, + * read_trip_file) and the predicate-gated tools carry no mode at all. + * tests/unit/mcp/demo-tool-gate.test.ts holds the two in step: every + * `mode: 'read'` tool is read-only and every `mode: 'write'` one is not. */ export function isDemoGatedTool(tool: ToolOptions): boolean { - return tool.annotations?.readOnlyHint !== true; + return tool.annotations?.readOnlyHint !== true && !DEMO_UNGATED_WRITE_TOOLS.has(tool.name); } /** diff --git a/server/src/nest-mcp/README.md b/server/src/nest-mcp/README.md index 514d0181b..b08060d3e 100644 --- a/server/src/nest-mcp/README.md +++ b/server/src/nest-mcp/README.md @@ -83,7 +83,8 @@ createTestRegistry(instances, { accessPolicy, toolGate: (tool, ctx) => undefined The gate is built from the container (`imports`/`inject`/`useFactory`), because a check that reads the database cannot live in static `forRoot` options. TREK's is `trekDemoToolGate` in `src/mcp/nest-mcp-policy.ts`: a demo account calling a -tool not annotated `readOnlyHint: true` gets `demoDenied()`. Tools therefore +tool not annotated `readOnlyHint: true` gets `demoDenied()`, except the few +write tools in `DEMO_UNGATED_WRITE_TOOLS` that never had a demo check. Tools therefore never check demo mode themselves; ESLint refuses `isDemoUser`/`isDemoUserId`/ `demoDenied()` in `*.mcp.ts`. diff --git a/server/tests/unit/mcp/demo-tool-gate.test.ts b/server/tests/unit/mcp/demo-tool-gate.test.ts index 6247b4135..46f69ee26 100644 --- a/server/tests/unit/mcp/demo-tool-gate.test.ts +++ b/server/tests/unit/mcp/demo-tool-gate.test.ts @@ -12,7 +12,7 @@ */ import { ADDON_IDS } from '../../../src/addons'; import { db as testDb } from '../../../src/db/database'; -import { isDemoGatedTool, trekDemoToolGate } from '../../../src/mcp/nest-mcp-policy'; +import { DEMO_UNGATED_WRITE_TOOLS, isDemoGatedTool, trekDemoToolGate } from '../../../src/mcp/nest-mcp-policy'; import type { McpContext, McpRegistry, McpRegistryListing } from '../../../src/nest-mcp'; import { withRequestContext } from '../../../src/nest/database/request-context'; import { createUser } from '../../helpers/factories'; @@ -93,12 +93,42 @@ describe('demo tool gate over the whole registry', () => { const { user: demo } = createUser(testDb, { email: 'demo@trek.app' }); const tools = await captureTools(registry, { userId: demo.id, scopes: null, isStaticToken: false }); - const writes = [...tools].filter(([, t]) => t.annotations?.readOnlyHint !== true); + const writes = [...tools].filter( + ([name, t]) => t.annotations?.readOnlyHint !== true && !DEMO_UNGATED_WRITE_TOOLS.has(name), + ); expect(writes.length).toBeGreaterThan(150); const answers = await Promise.all(writes.map(async ([name, tool]) => ({ name, result: await tool.call() }))); expect(answers).toEqual(writes.map(([name]) => ({ name, result: DEMO_REFUSAL }))); }); + it('DEMOGATE-007: the write tools that never had a demo check stay open to the demo account', async () => { + process.env.DEMO_MODE = 'true'; + const { user: demo } = createUser(testDb, { email: 'demo@trek.app' }); + const ctx: McpContext = { userId: demo.id, scopes: null, isStaticToken: false }; + const tools = await captureTools(registry, ctx); + const gate = trekDemoToolGate(async () => true); + const open = [...DEMO_UNGATED_WRITE_TOOLS].sort(); + expect(open).toEqual([ + 'create_manual_school_holiday_country', + 'create_manual_school_holiday_region', + 'delete_manual_school_holiday_country', + 'delete_manual_school_holiday_region', + 'sync_trip_documents', + 'update_manual_school_holiday_region', + ]); + for (const name of open) { + expect(tools.get(name), name).toBeDefined(); + expect(tools.get(name)?.annotations?.readOnlyHint, name).not.toBe(true); + } + const refusals = await Promise.all( + open.map(async (name) => ({ + name, + refusal: await gate({ name, annotations: tools.get(name)?.annotations }, ctx), + })), + ); + expect(refusals).toEqual(open.map((name) => ({ name, refusal: undefined }))); + }); + it('DEMOGATE-002: no read-only tool is held back for the demo account', async () => { process.env.DEMO_MODE = 'true'; const { user: demo } = createUser(testDb, { email: 'demo@trek.app' }); @@ -183,6 +213,14 @@ describe('trekDemoToolGate', () => { expect(isDemo).not.toHaveBeenCalled(); }); + it('DEMOGATE-014: a listed write tool passes the gate without asking about the caller', async () => { + const isDemo = vi.fn(async () => true); + const gate = trekDemoToolGate(isDemo); + expect(await gate({ name: 'sync_trip_documents', annotations: { readOnlyHint: false } }, ctx)).toBeUndefined(); + expect(isDemo).not.toHaveBeenCalled(); + expect(isDemoGatedTool({ name: 'delete_manual_school_holiday_country' })).toBe(false); + }); + it('DEMOGATE-013: a tool without annotations counts as a write', () => { expect(isDemoGatedTool({ name: 'x' })).toBe(true); expect(isDemoGatedTool({ name: 'x', annotations: { readOnlyHint: true } })).toBe(false);