diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8e23cd902..9c6187fcb 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -92,6 +92,14 @@ jobs: - name: Test the classifier run: npm run test:ci-scripts + - name: Guides only name paths that exist + # Every backtick path in the CLAUDE.md files, the root README, the Nest + # blueprint and the wiki has to resolve to a tracked file; the ones + # already missing are listed in scripts/ci/doc-paths-baseline.json, + # which only shrinks. Here rather than in a code job, so a docs-only + # change is checked too. + run: node scripts/ci/doc-paths.mjs + - name: List the changed files id: list env: diff --git a/CLAUDE.md b/CLAUDE.md index 95e03ed24..caf453e01 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,7 +77,7 @@ Nest owns everything: every domain is a DI module under `server/src/nest/')` per handler — and forward `X-Socket-Id` to the broadcast so the originating client doesn't echo its own change. **Never gate a multipart upload with a guard** (the client sees ECONNRESET instead of 403 — check in the handler). -- **DB** (`server/src/db/`): `better-sqlite3`, WAL, FK on. MikroORM's migrations (`db/migrations/`) are the only schema source, **including for tests** — the hand-kept `schema.ts`/`migrations.ts`/`seeds.ts` test builders are gone. Seeding is MikroORM's too (`db/seeders/`), applied at boot from `buildApp()`; the ORM shares the one better-sqlite3 connection (bound in `db/orm-driver.ts`) rather than opening its own. Queries go through repositories (`server/src/db/repositories/`) and `UnitOfWork` (`server/src/nest/database/unit-of-work.ts`) for transactions — there is no `DatabaseService`. `better-sqlite3` itself is ESLint-restricted to `src/db/` plus a short, named allow-list (`server/eslint.config.mjs`), so a domain reaching for the driver directly fails lint. `NODE_ENV=test` gives each vitest worker an isolated `:memory:` DB. +- **DB** (`server/src/db/`): `better-sqlite3`, WAL, FK on. MikroORM's migrations (`db/migrations/`) are the only schema source, **including for tests** — the hand-kept `schema.ts`/`migrations.ts`/`seeds.ts` test builders are gone. Seeding is MikroORM's too (`db/seeders/`), applied at boot from `buildApp()`; the ORM shares the one better-sqlite3 connection (bound in `server/src/db/orm-driver.ts`) rather than opening its own. Queries go through repositories (`server/src/db/repositories/`) and `UnitOfWork` (`server/src/nest/database/unit-of-work.ts`) for transactions — there is no `DatabaseService`. `better-sqlite3` itself is ESLint-restricted to `src/db/` plus a short, named allow-list (`server/eslint.config.mjs`), so a domain reaching for the driver directly fails lint. `NODE_ENV=test` gives each vitest worker an isolated `:memory:` DB. - **Realtime**: `/ws` is a Nest gateway in `server/src/nest/realtime/` — inject `RealtimeService`; `server/src/websocket.ts` is a legacy re-export stub. - **MCP**: tools are declared in `.mcp.ts` files beside each module, on the `server/src/nest-mcp/` decorator layer. A validation or permission change on a REST route must land in the parallel MCP tool in the same change. See `MCP.md`. - **Addons** are admin-toggleable feature modules keyed by `ADDON_IDS` in `server/src/addons.ts`. @@ -90,7 +90,7 @@ Nest owns everything: every domain is a DI module under `server/src/nest/()` hook; it must not call React state/effect/memo hooks itself. -- **Data flow**: `store (Zustand, client/src/store/) → repo (client/src/repo/) → api (client/src/api/) | Dexie (client/src/db/offlineDb.ts)`. Writes go through `sync/mutationQueue.ts`: optimistic Dexie write, then a replay with an `X-Idempotency-Key` header on reconnect. `store/slices/remoteEventHandler.ts` applies incoming WebSocket events. Components never call the API directly. +- **Data flow**: `store (Zustand, client/src/store/) → repo (client/src/repo/) → api (client/src/api/) | Dexie (client/src/db/offlineDb.ts)`. Writes go through `client/src/sync/mutationQueue.ts`: optimistic Dexie write, then a replay with an `X-Idempotency-Key` header on reconnect. `client/src/store/slices/remoteEventHandler.ts` applies incoming WebSocket events. Components never call the API directly. - **Styling**: semantic Tailwind tokens (`bg-surface*`, `text-content*`, `border-edge*`, `bg-accent*`) or the underlying `var(--token)` variables — never color literals, arbitrary-value color classes or numeric inline `fontSize`, so user-chosen scheme/transparency/text-size keep working. `theme:lint` flags bypasses; suppress intentional exceptions (map/PDF/brand) with a `theme-lint-disable` line comment. - **Reading direction**: Arabic runs the app right to left, so spacing, insets and alignment that follow the text use the logical forms (`ms-`/`me-`/`ps-`/`pe-`/`start-`/`end-`/`text-start`/`border-s`/`rounded-s`, `marginInlineStart`, `insetInlineEnd`, `textAlign: 'start'`). `lint:rtl` counts the physical ones per file against `client/scripts/rtl-baseline.json`, which may only shrink; geometry that is physical on purpose in new code (maps, measured positions, time axes) carries an `rtl-lint-disable` comment. - **PWA**: `vite-plugin-pwa` + Workbox cache tiles, API and uploads; `prebuild` generates icons. @@ -126,6 +126,7 @@ These principles come out of a full-repo audit and shape all new code: `.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:boundaries`, `lint:tx`, `lint:dialect` and `lint:test-sql`, the client's `lint:format`, `lint:layers`, `lint:offline`, `lint:dup`, `lint:pairs`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and the statements every repository method reaches with sample arguments against a Postgres service; the SQL errors per method and the list of methods it cannot measure are held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `format:check` on `shared/`. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. - **`ci-ok` is the required check.** The branch ruleset requires `ci-ok` (plus `lint` and `check-target` from their own workflows), and `ci-ok` needs every job above except the scan. It fails if any of them failed or was cancelled and passes if they passed or were skipped. A new gate job counts only once it is listed in `ci-ok`'s `needs:`. +- **Guides name paths that exist**: the `changes` job runs `scripts/ci/doc-paths.mjs`, which resolves every backtick path in the CLAUDE.md files, `README.md`, `server/src/nest/README.md` and the wiki against the tracked files; the missing ones are listed in `scripts/ci/doc-paths-baseline.json`, which only shrinks (`--update` after a fix). - **What runs is decided by `changes`, not by `on.paths`**: `scripts/ci/changed-areas.mjs` maps the changed files to the `code`, `image` and `deploy` areas (tests: `npm run test:ci-scripts`). A path filter on the workflow would leave `ci-ok` unreported on a docs-only PR and block it. Add a new top-level file the build reads to the right area there. - **Runners and Node**: jobs run on `ubuntu-24.04`, not `ubuntu-latest`, and every `setup-node` reads `.nvmrc`. Third-party actions (anything outside `actions/*`) are pinned by commit SHA with the tag in a comment; Dependabot keeps both current. - **Security scans are advisory.** `security.yml` runs `npm audit` (what the image installs, plus `plugin-sdk`) on every PR and push into dev and main, and Docker Scout on the built image for main. Neither is required: a newly published advisory would turn every open PR red at once. Fixes are held as floors in the root `package.json` `overrides`; npm ignores an `overrides` block in a workspace `package.json`, so never put one there. diff --git a/scripts/ci/doc-paths-baseline.json b/scripts/ci/doc-paths-baseline.json new file mode 100644 index 000000000..98ba3324b --- /dev/null +++ b/scripts/ci/doc-paths-baseline.json @@ -0,0 +1,30 @@ +[ + "client/CLAUDE.md :: slices/remoteEventHandler.ts", + "plugin-sdk/CLAUDE.md :: protocol/output-contract.ts", + "plugin-sdk/CLAUDE.md :: scripts/validate-entry.mjs", + "server/CLAUDE.md :: db/migrations.ts", + "server/CLAUDE.md :: host/plugin-data.service.ts", + "server/CLAUDE.md :: host/rpc-host.ts", + "server/CLAUDE.md :: pg-probe/arg-samples.ts", + "server/CLAUDE.md :: pg-probe/recorder.ts", + "server/CLAUDE.md :: protocol/envelope.ts", + "server/CLAUDE.md :: protocol/output-contract.ts", + "server/CLAUDE.md :: runtime/egress-policy.ts", + "server/CLAUDE.md :: runtime/plugin-host-entry.ts", + "wiki/Encryption-Key-Rotation.md :: scripts/migrate-encryption.ts", + "wiki/Environment-Variables.md :: scripts/migrate-encryption.ts", + "wiki/Environment-Variables.md :: templates/configmap.yaml", + "wiki/Environment-Variables.md :: templates/secret.yaml", + "wiki/Plugin-Development.md :: client/settings.html", + "wiki/Plugin-Development.md :: server/index.js", + "wiki/Plugin-Permissions.md :: docs/screenshot.png", + "wiki/Plugin-Permissions.md :: server/index.js", + "wiki/Plugin-Publishing.md :: docs/screenshot.png", + "wiki/Plugin-Publishing.md :: schema/example-entry.json", + "wiki/Plugin-Publishing.md :: schema/plugin-entry.schema.json", + "wiki/Plugin-Publishing.md :: scripts/check-readme.mjs", + "wiki/Plugin-Publishing.md :: scripts/validate-entry.mjs", + "wiki/Plugin-Publishing.md :: server/index.js", + "wiki/Plugins.md :: server/data/plugins", + "wiki/Plugins.md :: server/data/travel.db" +] diff --git a/scripts/ci/doc-paths.mjs b/scripts/ci/doc-paths.mjs new file mode 100644 index 000000000..2c04c1883 --- /dev/null +++ b/scripts/ci/doc-paths.mjs @@ -0,0 +1,179 @@ +#!/usr/bin/env node +// Every repository path a guide names in backticks has to exist. +// +// The CLAUDE.md files, the root README, the Nest module blueprint and the wiki +// are what a contributor (or an agent) reads before touching the code, and a +// path in them is an instruction: open this file, copy that module. When the +// file has moved or died, the guide sends the reader to nothing, or worse, to +// a recipe for a layer that no longer exists. So this reads each guide, +// takes every inline code span that looks like a path into the repository and +// checks it against the files git tracks. +// +// A span counts as a repository path when it has no spaces, globs, +// placeholders or URL parts, holds a slash, does not start with `/`, `~` or +// `@` (a route, a home path, a package name), and either ends in a file +// extension or starts with a directory that exists from one of the bases it +// is read against. A trailing `:line` or `:line-line` is ignored. The bases +// are the repository root, the guide's own directory, its workspace +// (`server/` for anything under server/) and that workspace's `src/`, so +// `src/nest/README.md` in server/CLAUDE.md and `nest/database/unit-of-work.ts` +// both resolve. Fenced code blocks are not read: they hold commands and +// examples, not references. +// +// The paths that were already missing when the check came in are listed in +// scripts/ci/doc-paths-baseline.json as " :: ". The list only +// shrinks: a missing path outside it fails, and so does a listed one that +// resolves now or whose guide is gone, until --update takes it off. +// +// node scripts/ci/doc-paths.mjs check against the baseline (CI) +// node scripts/ci/doc-paths.mjs --update take resolved entries off the list; +// it never adds one +// node scripts/ci/doc-paths.mjs --list print every missing path +// +// It needs nothing but Node and git, so it runs in the changes job before +// anything is installed. + +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import { join, posix } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { fileList, lowerList, readBaseline, runCli, writeBaseline } from '../lib/ratchet.mjs'; + +export const WORKSPACES = ['client', 'server', 'shared', 'plugin-sdk']; + +/** The guides the check reads, out of the tracked files. */ +export function guidesOf(tracked) { + return [...tracked] + .filter( + (path) => + path === 'README.md' || + path === 'server/src/nest/README.md' || + posix.basename(path) === 'CLAUDE.md' || + /^wiki\/[^/]+\.md$/.test(path), + ) + .sort(); +} + +/** The inline code spans of a Markdown text, outside fenced code blocks. */ +export function inlineSpans(markdown) { + const spans = []; + let fence = null; + for (const line of markdown.replace(/\r\n?/g, '\n').split('\n')) { + const marker = /^\s*(`{3,}|~{3,})/.exec(line); + if (marker) { + if (!fence) fence = marker[1][0]; + else if (marker[1][0] === fence) fence = null; + continue; + } + if (fence) continue; + for (const match of line.matchAll(/(? posix.normalize(base ? `${base}/${path}` : path).replace(/\/$/, ''); + +/** + * The span as a repository path to check, or null when it is not one. exists + * tells whether a path (file or directory, no trailing slash) is tracked. + */ +export function pathOf(span, bases, exists) { + const path = span.replace(/:\d+(?:-\d+)?$/, '').replace(/[.,;]$/, ''); + if (!path.includes('/') || /[\s*?<>{}[\]$|=()'"\\,]/.test(path)) return null; + if (/^[/~@]/.test(path) || path.includes('://') || /^[a-z]+:/i.test(path)) return null; + const first = path.replace(/^(?:\.\.?\/)+/, '').split('/')[0]; + const fromKnownDir = bases.some((base) => exists(join2(base, first))); + if (!EXTENSION.test(path) && !fromKnownDir) return null; + return path; +} + +/** Module specifiers are often written without their extension (`api/client`). */ +const IMPLIED = ['', '.ts', '.tsx', '.js', '.mjs']; + +/** Whether the path resolves from one of the bases, as written or as a module specifier. */ +export function resolves(path, bases, exists) { + return bases.some((base) => IMPLIED.some((ext) => exists(join2(base, path) + ext))); +} + +/** The missing paths of every guide, as " :: " entries. */ +export function missingPaths({ tracked, read }) { + const files = new Set(tracked); + const dirs = new Set(); + for (const file of files) { + for (let at = file.indexOf('/'); at !== -1; at = file.indexOf('/', at + 1)) dirs.add(file.slice(0, at)); + } + const exists = (path) => files.has(path) || dirs.has(path); + const missing = new Set(); + for (const guide of guidesOf(files)) { + const bases = basesOf(guide); + for (const span of inlineSpans(read(guide))) { + const path = pathOf(span, bases, exists); + if (path && !resolves(path, bases, exists)) missing.add(`${guide} :: ${path}`); + } + } + return [...missing].sort(); +} + +/** Runs the check against the list at baselinePath and returns the exit code. */ +export function check({ + root, + tracked, + baselinePath = join(root, 'scripts/ci/doc-paths-baseline.json'), + update = false, + list = false, + log = console.log, + error = console.error, +}) { + const found = missingPaths({ tracked, read: (path) => readFileSync(join(root, path), 'utf8') }); + let baseline = readBaseline(baselinePath, fileList); + if (update) { + baseline = lowerList(baseline, found); + writeBaseline(baselinePath, baseline); + } + const allowed = new Set(baseline); + const now = new Set(found); + const fresh = found.filter((entry) => !allowed.has(entry)); + const stale = baseline.filter((entry) => !now.has(entry)); + + if (list) for (const entry of found) log(`${allowed.has(entry) ? 'listed' : 'new '} ${entry}`); + for (const entry of fresh) { + const [guide, path] = entry.split(' :: '); + error(`FAIL ${guide} names \`${path}\`, which does not exist. Point it at the file that does, or drop it.`); + } + for (const entry of stale) { + error(`FAIL "${entry}" is listed in scripts/ci/doc-paths-baseline.json but resolves now or its guide is gone.`); + } + if (stale.length) error('Run node scripts/ci/doc-paths.mjs --update to take it off the list.'); + log(`doc-paths: ${found.length} missing path(s) in the guides, ${baseline.length} listed in the baseline`); + return fresh.length || stale.length ? 1 : 0; +} + +function trackedFiles(root) { + const out = execFileSync('git', ['-c', 'core.quotePath=false', 'ls-files', '-z'], { + cwd: root, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }); + return out.split('\0').filter(Boolean); +} + +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { + const root = fileURLToPath(new URL('../..', import.meta.url)); + await runCli('doc-paths', (args) => + check({ root, tracked: trackedFiles(root), update: args.includes('--update'), list: args.includes('--list') }), + ); +} diff --git a/scripts/ci/doc-paths.test.mjs b/scripts/ci/doc-paths.test.mjs new file mode 100644 index 000000000..387c7a655 --- /dev/null +++ b/scripts/ci/doc-paths.test.mjs @@ -0,0 +1,154 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { afterEach, describe, it } from 'node:test'; +import { basesOf, check, guidesOf, inlineSpans, missingPaths, pathOf, resolves } from './doc-paths.mjs'; + +const TRACKED = [ + 'CLAUDE.md', + 'README.md', + 'server/CLAUDE.md', + 'server/src/nest/README.md', + 'server/src/nest/weather/weather.service.ts', + 'server/src/nest/database/unit-of-work.ts', + 'server/src/config.ts', + 'server/tests/e2e/harness.ts', + 'client/src/api/client.ts', + '.github/workflows/test.yml', + 'wiki/Home.md', + 'wiki/assets/a.png', + 'docs/other.md', +]; + +const exists = (path) => { + const files = new Set(TRACKED); + return files.has(path) || TRACKED.some((file) => file.startsWith(`${path}/`)); +}; + +describe('doc-paths', () => { + it('reads the CLAUDE.md files, the root README, the Nest blueprint and the top-level wiki pages', () => { + assert.deepEqual(guidesOf([...TRACKED, 'wiki/sub/Deep.md', 'server/README.md']), [ + 'CLAUDE.md', + 'README.md', + 'server/CLAUDE.md', + 'server/src/nest/README.md', + 'wiki/Home.md', + ]); + }); + + it('takes inline code spans and skips fenced blocks of either kind', () => { + const md = [ + 'Open `server/src/config.ts` and `a b`.', + '```bash', + 'cat `server/nope.ts`', + '```', + '~~~', + '`server/also-nope.ts`', + '~~~', + 'Then ``double`` and `client/src/api/client.ts`.', + ].join('\r\n'); + assert.deepEqual(inlineSpans(md), ['server/src/config.ts', 'a b', 'client/src/api/client.ts']); + }); + + it('reads a relative path against the guide directory, its workspace and the workspace src', () => { + assert.deepEqual(basesOf('server/src/nest/README.md'), ['', 'server/src/nest', 'server', 'server/src']); + assert.deepEqual(basesOf('CLAUDE.md'), ['']); + assert.deepEqual(basesOf('wiki/Home.md'), ['', 'wiki']); + }); + + it('only treats a span as a path when it looks like one', () => { + const bases = basesOf('server/CLAUDE.md'); + assert.equal(pathOf('src/nest/README.md', bases, exists), 'src/nest/README.md'); + assert.equal(pathOf('.github/workflows/test.yml:250-251', [''], exists), '.github/workflows/test.yml'); + assert.equal(pathOf('nest/database/unit-of-work.ts,', bases, exists), 'nest/database/unit-of-work.ts'); + assert.equal(pathOf('src/gone/', bases, exists), 'src/gone/'); + for (const span of [ + 'weather.service.ts', + '/api/health/ready', + '~/.ssh/key', + '@trek/shared', + 'https://example.com/a.md', + 'text/plain', + 'src/**/*.ts', + '/.rpc.ts', + 'npm run lint', + 'a/{b,c}.ts', + 'C:/tmp/x.ts', + ]) { + assert.equal(pathOf(span, bases, exists), null, span); + } + }); + + it('resolves a path as written or as a module specifier without its extension', () => { + assert.equal(resolves('src/config', basesOf('server/CLAUDE.md'), exists), true); + assert.equal(resolves('api/client', ['', 'client/src'], exists), true); + assert.equal(resolves('weather/weather.service.ts', basesOf('server/src/nest/README.md'), exists), true); + assert.equal(resolves('src/nope.ts', basesOf('server/CLAUDE.md'), exists), false); + }); + + it('lists every missing path once per guide', () => { + const texts = { + 'CLAUDE.md': 'See `server/src/config.ts`, `server/src/gone.ts` and again `server/src/gone.ts`.', + 'README.md': 'Nothing here.', + 'server/CLAUDE.md': 'Use `nest/database/unit-of-work.ts` and `tests/e2e/harness.ts`, not `db/migrations.ts`.', + 'server/src/nest/README.md': '`weather/weather.service.ts` and `src/services/`.', + 'wiki/Home.md': 'Pictures live in `assets/a.png`; plugins ship `server/index.js`.', + }; + assert.deepEqual(missingPaths({ tracked: TRACKED, read: (path) => texts[path] }), [ + 'CLAUDE.md :: server/src/gone.ts', + 'server/CLAUDE.md :: db/migrations.ts', + 'server/src/nest/README.md :: src/services/', + 'wiki/Home.md :: server/index.js', + ]); + }); + + describe('check', () => { + let root; + afterEach(() => root && rmSync(root, { recursive: true, force: true })); + + function tree(files, baseline) { + root = mkdtempSync(join(tmpdir(), 'doc-paths-')); + for (const [path, text] of Object.entries({ ...files, 'scripts/ci/doc-paths-baseline.json': JSON.stringify(baseline) })) { + mkdirSync(dirname(join(root, path)), { recursive: true }); + writeFileSync(join(root, path), text); + } + const out = { log: [], error: [] }; + const run = (opts = {}) => + check({ + root, + tracked: Object.keys(files), + log: (l) => out.log.push(l), + error: (l) => out.error.push(l), + ...opts, + }); + return { out, run }; + } + + it('passes when every missing path is listed, and fails a new one', () => { + const listed = tree({ 'CLAUDE.md': '`server/a.ts`', 'server/b.ts': '' }, ['CLAUDE.md :: server/a.ts']); + assert.equal(listed.run(), 0); + rmSync(root, { recursive: true, force: true }); + const fresh = tree({ 'CLAUDE.md': '`server/a.ts` `server/c.ts`', 'server/b.ts': '' }, ['CLAUDE.md :: server/a.ts']); + assert.equal(fresh.run(), 1); + assert.match(fresh.out.error.join('\n'), /CLAUDE\.md names `server\/c\.ts`, which does not exist/); + }); + + it('fails a listed entry that resolves now until --update takes it off, and never adds one', () => { + const t = tree({ 'CLAUDE.md': '`server/b.ts` `server/new.ts`', 'server/b.ts': '' }, ['CLAUDE.md :: server/b.ts']); + assert.equal(t.run(), 1); + assert.match(t.out.error.join('\n'), /"CLAUDE\.md :: server\/b\.ts" is listed .* resolves now/); + assert.match(t.out.error.join('\n'), /--update/); + assert.equal(t.run({ update: true }), 1); + assert.deepEqual(JSON.parse(readFileSync(join(root, 'scripts/ci/doc-paths-baseline.json'), 'utf8')), []); + }); + + it('prints every missing path with --list, and stops on a broken baseline', () => { + const t = tree({ 'CLAUDE.md': '`server/a.ts`', 'server/b.ts': '' }, ['CLAUDE.md :: server/a.ts']); + assert.equal(t.run({ list: true }), 0); + assert.ok(t.out.log.includes('listed CLAUDE.md :: server/a.ts')); + writeFileSync(join(root, 'scripts/ci/doc-paths-baseline.json'), '{'); + assert.throws(() => t.run(), /not valid JSON/); + }); + }); +}); diff --git a/server/docs/nest-migration-history.md b/server/docs/nest-migration-history.md new file mode 100644 index 000000000..3a2d27114 --- /dev/null +++ b/server/docs/nest-migration-history.md @@ -0,0 +1,1002 @@ +# NestJS migration: history (historical) + +**Historisch / historical record.** This is the module guide as it stood while +the Express API was being strangled into Nest, kept for the reasoning behind +decisions that still shape the code (why a registry is module state, why a +bridge was folded the way it was, how each legacy service moved). It is not a +guide for new code: `src/services/`, every `*.bridge.ts`, `DatabaseService` +and the routing gate it describes are gone. The current blueprint is +`src/nest/README.md`; the rules are in `server/CLAUDE.md`. + +Paths below are relative to `server/src/nest/` unless they say otherwise, and +many name files that no longer exist. + +--- + + +This folder holds the co-hosted NestJS app that incrementally strangles the legacy +Express API (see the "Brownfield Rewrite" board). Until a prefix is migrated, the +top-level dispatcher in `src/index.ts` routes it to the legacy app; migrated +prefixes go to Nest. **Weather (`weather/`) is the reference implementation** — copy +its shape when migrating a new domain. + +## Module layout (per domain) + +``` +shared/src//.schema.ts(.spec.ts) # Zod contract — single source of truth +server/src/nest//.service.ts # business logic (ported 1:1 from the Express service) +server/src/nest//.controller.ts # same routes/verbs/params/status codes as Express +server/src/nest//.module.ts # registered in app.module.ts +``` + +Register the module in `app.module.ts` and it is live. There is no routing gate to +flip any more: **every request goes through Nest**. The `strangler.ts` matcher and +its `DEFAULT_NEST_PREFIXES` / `NEST_PREFIXES` escape hatch are gone — Express only +remains as the platform underneath `@nestjs/platform-express`. + +## Migrated so far + +- **Phase 1 (leaf):** weather, airports, config (public), system-notices, maps, + categories, tags, notifications, atlas. +- **Phase 2 (trip sub-domains):** vacay (addon), packing, todo. +- **DI-native services (legacy `src/services/*` deleted):** tags, categories, + todo, packing, day-notes, trip-invite, assignments, share, settings, files, + collab, vacay, reservations, day, permissions, audit, budget, trip, maps, + transit, place, transit-itinerary, collections, atlas, auth, oidc, passkey, + notifications, admin, webauthn-config, user-cleanup, oauth, wiki, mailer, + notification transports, notification preferences, memories (immich, synology, + unified, photo-resolver, thumbnails, trek-photo cache), journey, journeyShare + — see the migration recipe below. +- **Foundation (BE-Phase 1, complete):** the eight stateless helpers moved to + `common/`; every trip-access check routes through a repository + (`services/tripAccess.ts` and the dead `middleware/tripAccess.ts` are gone; + `DatabaseService`, the facade BE-Phase 1 originally routed this through, was + itself retired by the later ORM migration — see the root/server `CLAUDE.md`); + `queryHelpers` and `tripMembership` are providers; every cron is a domain + `*.job.ts` provider on `scheduling/CronRegistrarService` (`src/scheduler.ts` + is deleted). +- **Plugin RPC surface (BE-7, complete):** all 113 wire methods are + `@PluginMethod` / `@PluginOpenMethod` handlers on `@PluginController()` providers + in their own domains (`/.rpc.ts`; the three that belong to no + domain live in `plugins/host/rpc/`), and all 18 host-to-plugin hook contracts are + `@PluginHook` declarations on `PluginHooks`. `PluginRpcHost` no longer holds a + handler: it builds the granted subset of the dispatch map from the registry, + dispatches, audits and maps errors. `PluginHostDepsFactory` — the 26-argument + wiring sheet named throughout the log below — is now + `PluginRpcHostFactory` and injects two things. See + `plugins/host/rpc-kit/README.md`. +- **Plugin module split (complete):** `PluginsModule` was 21 controllers, 24 domain + imports and 10 providers in one class, so `AdminModule` importing it for one + cascade-disable call inherited the whole graph. It is now `PluginsRuntimeModule` + (supervisor, capability router, hook contracts — the half with the domain imports), + `PluginContributionsModule` (the 14 read-only hook controllers, in + `plugins/contributions/`), `PluginOAuthModule` (`plugins/oauth/`) and a composition + root holding the install and delivery controllers. `AdminModule` imports the runtime + alone. Note the invariant the boot-time coverage check now depends on: importing + `PluginsRuntimeModule` must pull in EVERY domain that owns part of the wire surface, + which is why `WeatherModule` sits in its import list. +- **Guards (BE-Phase 2, done):** the JWT verify lives in `auth/`, and + `middleware/auth.ts`, `validate.ts`, `idempotency.ts` and `mfaPolicy.ts` are + deleted; the three addon guards are one `AddonGuard` + `@RequireAddon`; the + demo-mode block is one condition. `TripAccessGuard` and `TripOwnerGuard` cover + the trip-scoped and owner-only routes. + + **Authentication is default-deny.** `GlobalAuthGuard` is an `APP_GUARD`: a route + is authenticated unless it carries `@Public(reason)` or `@OptionalAuth(reason)`, + or declares its own `@UseGuards` chain. A controller that declares a chain keeps + it — global guards run before route guards, so an addon-gated controller has to + stay in charge of answering 404 before anything answers 401, or the response + leaks which addons are installed. The global guard still *resolves* the user in + that case, without refusing, because `MfaPolicyGuard` runs behind it and reads + `req.user`. + + Adding a `@Public()` route is not a local decision: `validateRouteGuards` runs in + `buildApp()` and refuses to boot unless the route is also in + `PUBLIC_ROUTE_ALLOW_LIST` in `common/validate-route-guards.ts`. That list is the + whole anonymous surface of the server, in one reviewable place. Stale entries + fail too. + +**`src/services/` is gone.** It was the legacy layer this whole migration existed +to empty, and the last of it — `airtrail/`, 1305 lines of plain functions over the +better-sqlite3 singleton — folded into `integrations/`. An ESLint +`no-restricted-imports` rule now refuses any import of a `services/` path, because +the directory disappearing is not what keeps it gone: it grew one reasonable file +at a time. Backup made the same trip earlier: `services/backupService.ts` is +`nest/backup/backup.impl.ts`, imported only by its own domain. It is still a +module of free functions, but it no longer closes or reopens the core DB handle +itself: `BackupService` hands it the injected database backup port +(`DATABASE_BACKUP`, `SqliteDatabaseBackup`), which snapshots and swaps the file +through `DatabaseLifecycle`, the provider that owns the connection. The demo +reset goes through the same port. Another engine plugs in there, with a logical +dump in place of `VACUUM INTO` and the file swap. + +Note that the trip-access and `canEdit` methods on the domain services are **not** +dead weight waiting for a guard: their callers are overwhelmingly the `*.mcp.ts` +tools, which never pass through an HTTP guard. In the five domains piloted for +`TripAccessGuard`, 40 of the 46 callers are MCP; only 6 sit in controllers. + +## Cross-cutting Foundation pieces + +- `common/idempotency.interceptor.ts` — global `APP_INTERCEPTOR` replaying the + client's `X-Idempotency-Key` on mutations. It replaced the `applyIdempotency` + middleware, which is deleted; this is the only implementation. +- `common/` — the stateless helpers (`avatarUrl`, `conflictResult`, `demo`, + `passwordPolicy`, `timezoneService`, `cookie`, `rowShape`, `geo`, `crypto/`). + Free functions, not providers: the numbered migrations in `db/migrations/` + (e.g. the plaintext-secret encryption steps) and `demo/demo-seed.ts` import + them from outside the container and could not inject one. `geo.ts` holds the two haversines; there + were three implementations of the metre variant, differing by one clamp, and + the clamped form is the one kept — `asin` of a value a hair over 1 is NaN, and + a NaN distance silently fails every comparison it feeds instead of throwing. + `maps.helpers.ts` re-exports `haversineMetres` rather than defining it, because + callers and a test import it from there. +- `auth/jwt-verify.ts` — `extractToken` + `verifyJwtAndLoadUser`, the canonical + session check behind the four guards, the MCP bearer path and the file-download + query token. Free functions for the same reason. + `JWT_SECRET` stays a live binding from `src/config` and must NOT move to + `app-config`: the admin panel rotates it at runtime, and a `registerAs` token + would freeze the boot value. +- `query-helpers/` — the batch loaders that keep list endpoints off N+1 queries. +- `trip-membership/` — `joinTripAsMember`. Its own module rather than folded into + auth or trip-invite, because both call it (as do oidc and the plugin host) and + either home would put `AuthModule` and `TripInviteModule` in a cycle. +- `addons/addon.guard.ts` + `@RequireAddon(addonId, label)` — the addon route + gate. Declare `AddonGuard` **before** `JwtAuthGuard`: a disabled addon has to + answer 404 to anonymous callers, so the addon check must beat the 401. +- `common/demo-write.ts` — `isDemoWriteBlocked`, the demo-mode upload block. A + function and not a guard on purpose; see the gotcha below. +- `storage/` — the storage abstraction and its admin surface. `storage.types.ts` + defines the driver contract (opaque keys, no rename/append — S3 constraints + baked in) plus the `ServedCategory` seam: `STORAGE_CATEGORIES` (shared, 8, + configurable) + the legacy `photos` directory that is served and backed up + but not configurable. `drivers/` (local, s3, mirror) all pass one contract + suite (`tests/unit/nest/storage/storage-driver.contract.ts`). + `storage-registry.service.ts` merges built-ins ← env (`place-photos-local`) + ← `app_settings` rows, validates semantics (references, no mirror nesting), + falls back to last-good/defaults at boot but NEVER on admin writes + (`preview()` runs the real build first), seeds once from + `data/storage-config.json`, and hot-swaps on `reload()`. + `storage.service.ts` is the facade every byte-path uses. + `storage-admin.{controller,service}.ts` (+ `storage-secrets.ts`, + `storage-probe.ts`) carry `api/admin/storage`: masked GET of the effective + world, full-document PUT (unmask → preview → encrypt → one transaction → + reload → audit), and per-target test probes with ephemeral drivers. Secrets + live `enc:v1:`-encrypted inside the `storage.backends` JSON (the implicit + key covers installs without an explicit `ENCRYPTION_KEY`). The client + presents mirrors as replicas-on-primary; the wire keeps explicit mirror + backends. `storage-jobs.service.ts` runs backfill and category-migration + jobs (one at a time, TTL'd statuses). +- `memories/photo-provider.ts` + `providers/` — the PhotoProvider seam (#584). + Dispatch to a photo backend was a `switch (photo.provider)` in three places + (`photo-resolver.service.ts` twice, `journey-public.controller.ts` once, where + anything that was not Immich got handed to Synology). It is + `PhotoProviderRegistry.get(id)` now, over adapters registered under the + `PHOTO_PROVIDERS` multi-provider token in `memories.module.ts` — adding a + backend is one registration. + - Adapters, not `implements` on the services: ImmichService and + SynologyService are also the album/browse surfaces, with their own argument + orders and return shapes. The mapping lives in the adapter. + - `PhotoAssetRef` exists because the two services took the same three values + in different orders — `(userId, assetId, ownerId)` against + `(userId, targetUserId, photoId)`. A transposition compiled cleanly and + served another user's photo. + - `local` is deliberately NOT a provider. It is storage on this disk, it has + no row in `photo_providers`, and its "asset info" comes from the DB row + rather than from a backend. Both dispatch sites branch on it explicitly. + - The rename `memories/` → `media/` was considered and dropped: it is 275 + files of import churn, and `scripts/coverage-thresholds.mjs` buckets on one + path segment, so merging `photos/` into it would collapse two ratchet + entries into one and drop the stricter of the two. +- `realtime/` — the /ws transport, as a Nest gateway. + - `realtime.gateway.ts` owns the connection lifecycle and the handshake, with + `TripsRepository`/`UsersRepository`/`AppSettingsRepository` and + `EphemeralTokenService` injected instead of imported (Plan 4 converted it + off the deleted `DatabaseService`). It replaced `setupWebSocket(server)`, + which index.ts kicked off with a dynamic import after listen(). + - `ws-state.ts` holds the socket registry, and that is **module state on + purpose**. The no-Nest test harnesses build `new RealtimeService()` + outside the container and 115 test files `vi.mock` the `src/websocket` + stub; rooms on a provider instance would leave their broadcasts going to + an empty map, with no error and no log. Same reasoning as the geo + throttle cursor. + - `trek-ws.adapter.ts` exists because the stock `WsAdapter` dispatches on + `{ event, data }` and every deployed client sends `{ type, tripId }`. It + also builds the ws server itself rather than delegating: the base's port-0 + branch uses `noServer: true` and performs the upgrade from its own registry, + which never consults `verifyClient`, so delegating would silently drop the + origin check. The per-socket `error` listener is attached FIRST, because an + unhandled one kills the process (#1576), and the flood guard counts every + inbound frame before parsing, because malformed frames are the ones worth + limiting. + - The socket id stays a **monotonic integer**. The client echoes it as + `X-Socket-Id` and `broadcast` excludes the originator with + `Number(excludeSid)`; a uuid is `NaN`, `NaN === NaN` is false, and every + client would receive its own writes back. It shows up as drag-and-drop that + jumps, not as an error. + - `RealtimeGatewayModule` is imported by AppModule alone. RealtimeModule is + @Global, so a gateway there follows into every hand-assembled TestingModule, + and Nest's SocketModule then loads an adapter none of them set: it reaches + for `@nestjs/platform-socket.io` and calls `process.exit(1)` when it is + absent. + - `buildApp()` creates the http server and binds the adapter to it BEFORE + `app.init()`, and callers take it from `getHttpServer()`. Nest binds gateways + during init, so an adapter registered later binds nothing, and one given the + app instead of the server binds to Nest's internal server, which this process + never listens on. Either way the boot is green and no browser can connect. + - `src/websocket.ts` survives as a re-export. 115 test files `vi.mock` that + path to assert on broadcasts; repointing them all would be a large diff that + proves nothing. + +- `geo/` — the ONE Nominatim client (#576). There used to be two: atlas + throttled to the published 1.1s and cached its answers, maps did neither, so + five interactive routes fired straight at the service while the atlas loops + politely waited and OSM saw a single instance ignoring its rate limit. Both + domains now go through `nominatimFetch`. + - The throttle cursor and the cache are **module state, not provider state**, + for the permissions-cache reason: the no-Nest test harnesses build their + collaborators with `new`, outside the container, and a second copy of the + cursor is the very bug the fold removes. `GeoModule` is registered straight in AppModule and neither + `MapsModule` nor `AtlasModule` imports it — an import would claim an + injection edge that does not exist. + - `GeocodingService` owns exactly one thing: the cache-cleanup timer's + lifecycle, via `OnModuleInit`/`OnModuleDestroy`. Atlas used to start that + interval as an import-time side effect, documented as a parity exception. It + is not one any more. + - Two lanes over one cursor. Interactive calls (`/api/maps/search`, + `/autocomplete`, `/details`, `/reverse`, `/resolve-url`) take the next slot; + background calls (the atlas enrichment loops, which walk every uncached + place) yield to anything queued on a request path. + - The cache key carries the **query shape**, not coordinates alone. Atlas asks + at zoom 3 and zoom 8, maps at zoom 18; a coordinate-only key answered a + street lookup with a country. + - `AbortSignal.timeout` is built **after** the throttle wait. Built up front, + the 1.1s spent queueing would eat nearly half of the identity lookup's 2.5s + budget before the socket opens. + - `setGeoThrottleInterval(ms)` is a test seam, and `tests/setup.ts` zeroes it. + Nineteen maps cases drive these paths back to back with real timers; at the + real interval they slept nineteen seconds and covered nothing extra. A + function rather than a `NODE_ENV` check, for the reason app-config exists. + +### Reaching the container from outside it + +Two shapes, and the choice is settled: + +**Prefer handing the dependency in.** `bootstrap.ts` hands the `/mcp` handler +its registry via `setMcpRegistry(app.get(...))` after `buildApp()` — the +outside code names the capability it needs, structurally, never the provider +class, so no module cycle can close through it. (The retired `src/scheduler.ts` +was the fullest example — a structural `SchedulerDeps` interface filled by +`index.ts` — until every cron became an in-container `*.job.ts` provider and +the seam disappeared entirely, which is the real endgame for this shape.) + +**`.bridge.ts` is a retired shape** — zero bridge files remain since +the MCP/OAuth mount moved behind the container (2026-08-11). It served code +that ran before `app.init()` and so could not inject; its cost was real: a +bridge rebuilt its services with `new`, so adding one constructor parameter to +a service meant hand-editing every bridge that constructed it — five of them in +one PR, at the worst. If a pre-init consumer ever reappears, prefer the +`bootstrap.ts` pattern instead: resolve the container singleton with +`app.get(...)` before init (the `httpConfig.KEY` / discovery-metadata +precedent) rather than minting a second instance. + +**Express middleware with DI has two honest mounts** (first used by the mount +migration): `consumer.apply(...).forRoutes('')` in a module's +`configure()` — an Express prefix mount, exactly the legacy `app.use('/path')` +semantics, which is how `OauthModule` mounts the MCP SDK's authorize/register +routers and `PlatformModule` the consent-COOP override — and, for middleware +that must see the ORIGINAL `req.url` (a wildcard `forRoutes()` is a pattern +mount that strips the matched prefix), a pathless `app.use` in `bootstrap.ts` +over a container-resolved factory provider (`MCP_METADATA_MIDDLEWARE`). +`nest/mcp-transport/` (the /mcp controller + service over injected services) +and `nest/mcp-shared/` (`McpToolGuardsService` for the `*.mcp.ts` classes) are +the module shapes that replaced the last bridges. + +**A bridge imported from INSIDE the container hides an edge from the module +graph**, which is a different problem from serving a genuine outside consumer. +Nine `*.mcp.ts` files did that with `addons.bridge` for one boolean each, and +not by preference: a decorator's options object is built while the class is +being defined, so the `when:` closure had no `this` to reach an injected service +through. `src/nest-mcp/` hands the gate its declaring instance now, and +`addons/addon-gate.ts` turns that into `addonGate(ADDON_IDS.X)` over an injected +`AddonsService`. `reservations.mcp.ts` (assignments) and `atlas.mcp.ts` / +`journey.mcp.ts` (auth) went the same way. + +No in-container bridge uses are left. The last four folded 2026-08-11, and +their folds are the playbook for any edge that reappears — plain injection +would have closed a real cycle in every case, so each cut the knot with a +finer-grained module instead: + +- `budget.mcp.ts` / `packing.mcp.ts` / `costs.rpc.ts` → `trips.bridge`: + everything that owns the hydrated trip reads (TripsService, + TripReadModelService, TripMembersService) lives in modules importing the + budget/packing domains, so the consumers moved instead — the id-level + membership reads went to the dependency-free `TripMembershipModule` + (`getOwnerId` / `listMemberUserIds` / `listAccessibleTripIds`) and the two + cross-domain prompts moved above the read model onto + `trips/trip-prompts.mcp.ts`. `trips.bridge` (102 lines, ~25 hand-built + services) is deleted. +- `places.mcp.ts` → `assignments.bridge`: the cycle `DaysModule → + PlacesModule → AssignmentsModule → DaysModule` is real, but only + AssignmentsMcp needs DaysService — the service itself does not, so it moved + to a leaf `AssignmentsDomainModule` (the journey-domain split) that + PlacesModule imports. `assignments.bridge` is deleted. +- `reservations.controller.ts` → `airtrail.bridge`: the pull genuinely needs + ReservationsService, so `AirtrailModule` keeps it — but the write-back push + only needs the hydrated reservation READ. That read became + `ReservationsReadService` (leaf, the trek_photos pattern), the link + lifecycle + push became `AirtrailLinkService` in `AirtrailCoreModule`, and + the controller injects it. `airtrail.bridge` is deleted, and + `notifications.instance.ts` — whose last consumer it was — died with it. + +Two more were on this list and folded 2026-08-11: + +- `auth/user-cleanup.service.ts` → `budget.bridge` died when `BudgetModule` + dropped its `AuthModule` import (BudgetMcp's demo guard reads + `RuntimeEnvService` + the users table via `isDemoUserId`, not + `AuthService`), which let `AuthModule` import `BudgetModule` and + `UserCleanupService` inject `BudgetService`. `budget.bridge` is deleted. +- `backup/backup.impl.ts` → `permissions.bridge`: the restore path is free + functions, not a provider, so there is nothing for an injected service to + hang off — but the thing it needs is the cache, not the service. The cache + moved to `permissions/permissions-cache.ts` (the `oauth.pending-codes.ts` + precedent) and `backup.impl` imports the plain `invalidatePermissionsCache` + from there. + +`files.bridge` used to be on the list and is gone, resolved 2026-08-10 exactly +as predicted: `MulterModule.registerAsync` over an `AllowedFileTypesService` +leaf, and `files.controller.ts` / `journey.controller.ts` build their multer +options from that. `systemNotices/conditions.ts` similarly stopped importing +`addons.bridge`: the enablement check is threaded in through +`ConditionContext.addonEnabled` by the DI-native `SystemNoticesService`. + +`days`, `packing`, `photos` and `reservations` had no production consumer left +and are deleted. Each still built a service outside the container at module load +for nobody, and three domain docblocks were still sending readers to them. The +cases that exercised them kept their assertions and point at the service. +`journey.bridge` died 2026-08-11 with `src/mcp/resources.ts`, when the last +four legacy resources became `@Resource`/`@ResourceTemplate` methods on +`journey.mcp.ts`. + +Five bridges remain, all pinned by the one pre-`app.init()` MCP/OAuth mount +(`addons`, `audit`, `auth`, `oauth`, `permissions` — shrunk to the exports that +seam consumes) and all dying together if that mount ever moves behind the +container. When one loses its last outside-container consumer, delete it in +the same change rather than leaving it as a courtesy: an unused bridge reads +as a supported entry point. + +The nine fire-and-forget notification senders inject too. They were lazy +`import('../notifications/notifications.bridge').then(({ send }) => …)` calls +working around a cycle that no longer exists, and the laziness hid the edge +while handing each send a NotificationsService built outside the container. + +That sweep needed a method, because `tests/` sits outside `tsconfig`'s `include`: +a missed constructor argument does not fail to compile, it arrives as +`undefined`. Adding `tests` to a throwaway `tsconfig` and diffing the errors +before and after named all **46** hand-wired `new` sites exactly; a grep would +have been a guess. Worth repeating for any change that widens a service +constructor — and worth knowing that such a config reports **288 pre-existing +errors** in the suites, almost all deliberate partial constructions in the +harnesses (`new AuthService(db, permissions, atlas)` where the class takes six). +That is why `tests/` is not in the build's `include`, and cleaning it up is its +own piece of work. + +`notifications.instance.ts` held the out-of-container `NotificationsService` +for the cycle-dodge bridges and died 2026-08-11 with the last of them +(`airtrail.bridge`) — every production consumer injects the container +singleton now. The module-scoped channel registry it leaned on stays (the +no-Nest test harnesses still construct instances; CHOVR-015 pins the +sharing). +- `app-config/` — the `@nestjs/config` binding (`AppConfigModule`, global). Never + read `process.env` in a module (ESLint enforces this): inject a boot-stable + namespace via its `registerAs` token (`@Inject(storageConfig.KEY) … ConfigType<…>`) + or read runtime-toggled values live through `RuntimeEnvService` / `readEnv()` + from `src/app-config`. Every variable has exactly one owner, a token or + `readEnv()`, and `config-ownership.test.ts` fails on a variable read through + both. A provider that reads a data path takes `DataPathsService` + (`AdminService`, `StorageRegistryService`, and through it the storage + jobs); plain helper modules call `resolveDataPaths()` (see + `src/app-config/README.md` for which and why). The classification and + invariants live in `src/app-config/README.md`. + +## Parity gotchas worth remembering + +- A POST that answers with `res.json` in Express stays **200**; add `@HttpCode(200)` + (Nest defaults POST to 201). Creates that Express sends as 201 need nothing. +- Static sub-routes that collide with a `:id` param (e.g. `/in-app/all` vs + `/in-app/:id`, `/reorder` vs `/:id`) must be declared **before** the param route. +- Reproduce bespoke admin/error wording exactly — e.g. notifications' `test-smtp` + returns `{ error: 'Admin only' }`, not the AdminGuard's `Admin access required`. +- Trip-scoped routes verify trip access (404) and the relevant permission (403) + per handler and forward `X-Socket-Id` to the WebSocket broadcast. +- **Never gate a multipart upload with a guard.** Guards run before the parser, + so throwing from one leaves the request body unread and Node resets the + connection — the client sees `ECONNRESET`, not your 403. Check inside the + handler, after the interceptor has drained the upload. `PROFILE-015` is the + regression test. +- **`@Global()` only applies to modules that are in the graph.** An e2e + `TestingModule` built around one domain module does not get `AppConfigModule` + for free just because it is global: a provider injecting `RuntimeEnvService` + fails to resolve and takes every suite importing that module down with it. + Import it explicitly, the way `PermissionsModule` is handled. +- **A module-scoped registry that outside code writes into must stay a module, + not a provider.** The notification channel registry is the live example: + `PluginRuntimeService` pushes its channel getter in at `onModuleInit`, and + `notifications.instance.ts` builds its own `NotificationsService` for the + surviving cycle-dodge bridges. As a provider that instance would get a second, + empty registry and every plugin channel would go quiet — with no error + anywhere. `CHOVR-015` is the regression test. Same reasoning as + `oauth/oauth.pending-codes.ts`. +- **Self-registration by side-effect import is a trap.** The built-in channels + used to register because `notificationPreferencesService` happened to `import + './notifications/builtins'`. Dropping that one line would have silenced email, + webhook and ntfy without a failure. They are built from injected transports and + registered in `NotificationsService`'s constructor now, so every path that can + dispatch has them. +- **The coverage gate reads `src/nest/**` and nothing else.** Moving code in from + `src/mcp/` or `src/services/` does not just relocate it — it starts being + measured. PR #1844 landed `airports.mcp.ts` at 0% branches because + `search_airports`/`get_airport` had never had a test in either world, and the + aggregate fell to 79.86% with every suite green. Before pushing anything that + moves files into this tree, run `npm run test:coverage`, not just `npm test`. +- A guard with a constructor dependency has to be a registered provider + everywhere it is used. The three auth guards are dependency-free on purpose — + 38 directories apply them and 21 do not import `AuthModule`, so giving them a + dependency is its own migration, not a side effect of another change. + +## Parity is law + +A migrated route must be **byte-identical** for the client: same URL, method, +query/body, HTTP status, `Set-Cookie`, and JSON body — including bespoke error +strings. Where the legacy route returns a hand-written error (e.g. weather's +`{ error: 'Latitude and longitude are required' }`), reproduce that exact body in +the controller rather than relying on the generic `ZodValidationPipe` envelope. + +## TripAccessGuard and TripOwnerGuard, and where they do not fit + +`permissions/trip-access.guard.ts` resolves `:tripId` once per request, answers 404 +"Trip not found" for anything the user cannot reach (never 403 — that would confirm the +id exists), and hands the row to the handler through `@Trip()`. `@RequirePermission` +carries the same action string the domain services pass to `checkPermission`. + +Two limits are worth knowing before rolling it onto another controller, because both +were found the expensive way: + +- **A guard runs before the body pipe.** Any route whose DTO validation is expected to + answer 400 *ahead of* the trip 404 cannot use it. `places` is exactly that case — its + e2e suite documents the pipe-first ordering as a deliberate parity shift — so its + create/update/import routes keep their in-handler `requireTrip`, and that is not an + oversight. Its five body-free routes (list, get, unrate, image, delete) do carry + the guard; the split runs along "does this handler take a DTO-typed body", not + along the domain. +- **A guard also runs before interceptors.** On a multipart upload route, a 404 sent + while the client is still streaming destroys the socket, so the caller sees + ECONNRESET instead of the 404. The three upload routes (files, collab note files, + places) keep their own check for that reason; their controllers apply the guard per + handler instead of on the class. + +`permissions/trip-owner.guard.ts` is the stricter sibling: `@RequireTripOwner(message)` +demands that the caller *owns* the trip, not merely that they may edit it. Handing a +trip over and creating or deleting guests are the two things a collaborator must never +do however generous the trip's permission settings are. + +It deliberately does **not** inject `PermissionsService`. `checkPermission` returns true +for every admin, and the trip actions are admin-lowerable, so routing ownership through +it would quietly hand any admin the ability to transfer other people's trips. The check +is the literal one the routes did by hand: `trip.user_id === user.id`. The message +travels in the metadata because the two call sites answer with different strings and +both are asserted. + +The services keep `verifyTripAccess`/`canEdit` regardless: most of their callers are +`*.mcp.ts` tools, which never pass through an HTTP guard. + +## Coverage is gated per domain, as a ratchet + +`server/vitest.config.ts` carries one threshold entry per `src/nest//`, set at +that domain's measured coverage minus one point. The single repo-wide 80% it replaced +let a well-covered domain subsidise a thin one — `booking-import` sits at 50% and +`integrations` at 20%, and the aggregate still cleared the bar, so either could have +lost another ten points with the build staying green. + +Regenerate the block with `node scripts/coverage-thresholds.mjs` after a run that +RAISED coverage. Raise an entry when you improve a domain; never lower one to make a +build pass. The script reads `coverage/coverage-summary.json` rather than the text +reporter on purpose: the text reporter prints one row per DIRECTORY, so a domain with +subdirectories reads several points higher there than it actually is. + +## How to write the tests + +Every module ships two kinds of tests. The coverage gate (`vitest.config.ts`) +requires ≥80% over `src/nest/**` — note that it is an **average across the whole +tree**, not a per-file floor, so a large untested module can hide behind the +well-covered small ones. + +1. **Service / controller unit spec** — `tests/unit/nest/.controller.test.ts`. + Instantiate the controller with a mocked service; assert status codes, the exact + `{ error }` bodies, and that inputs are forwarded correctly (defaults, coercion). + See `weather.controller.test.ts`. + + Parity against the behaviour being replaced belongs here too — assert the exact + status codes and bodies the old path produced. There is no separate + `tests/parity/` directory; it was removed along with the routing toggle. + +2. **e2e** — `tests/e2e/.e2e.test.ts`. Boot the Nest module against a temp + in-memory SQLite db via the shared harness (`tests/e2e/harness.ts`: + `createTempDb`/`seedUser`/`sessionCookie`), exercising the **real** `JwtAuthGuard` + end-to-end (401 without cookie, 200 with a signed session). Mock external I/O + (HTTP/etc.). See `weather.e2e.test.ts`. + +## Definition of Done (per module) + +Contract in `@trek/shared` → service ported 1:1 → controller with identical routes → +validation/error parity → unit + e2e tests, with the old behaviour asserted in the +unit test → module registered in `app.module.ts` → **then** decommission the legacy +service (separate step) → frontend points at the typed contract (Frontend Track). + +## Migrating a legacy `src/services/*` service into its Nest module (recipe) + +**Historical record.** `src/services/` finished emptying in 2026-08 (see above); +the numbered steps below describe how each fold was actually done at the time, +including its `DatabaseService` construction — a deliberate changelog, not a +recipe to follow for new code. `DatabaseService` itself is gone (ORM migration +Plan 4): a service now takes the repositories it needs plus `UnitOfWork` for +transactions, per the root and server `CLAUDE.md`'s DB sections. + +Pilot: **tags** (`services/tagService.ts` → `nest/tags/tags.service.ts` + +`nest/tags/tags.bridge.ts`); categories followed the same shape (and piloted the +first `@Resource` in `categories.mcp.ts`); todo followed too (and piloted the +first `@ResourceTemplate` plus the `when` addon gate in `todo.mcp.ts`, and the +first in-container consumer wiring: `TripsService` injects `TodoService` via +`exports: [TodoService]` instead of using the bridge); packing followed (the +largest port so far: a 17-tool + 2-resource `packing.mcp.ts` with inline admin +gates, and the first `PluginHostDepsFactory` swap done as part of a service +migration — no bridge entry for the plugin host); day-notes followed (the first +migration needing **no bridge at all**: after its three tools + resource moved +to `day-notes.mcp.ts` and the plugin host injection, nothing outside the +container consumed it); trip-invite followed (the smallest port: no MCP +surface, no plugin-host import and no bridge — the SQL folded straight into +`trip-invite.service.ts`); assignments followed (a 7-tool `assignments.mcp.ts`, +the plugin-host swap, and a bridge kept only for the two legacy registrars — +places and reservations — that borrow its existence checks; the batch loaders +stay in `services/queryHelpers.ts`, shared with the unmigrated place +service); share followed (never imported by the plugin host, and its three MCP +tools stay in the legacy trips registrar — their `trips:share` scope gate has +no declarative `access: { group, mode }` equivalent — so the port is the SQL +fold plus a 3-export `share.bridge.ts` for `mcp/tools/trips.ts`); settings +followed (no MCP surface, no bridge, and the first migration that converted an +in-container plain-function consumer into a provider instead of bridging it: +`llm-parse/llm-config.resolver.ts` became the injectable `LlmConfigResolver`, +injected by `LlmParseService` and `PluginHostDepsFactory`); files followed (no +MCP surface and no addon gate; the load-time constants + pure helpers consumed +by three controllers' module-scope multer configs moved to +`files.constants.ts`, and `files.bridge.ts` survives with a single export — +the request-time `getAllowedExtensions` app_settings read those configs need +outside DI — while every function consumer, including the plugin host and +`TripsService`, injects `FilesService`); collab followed (a 12-tool + +3-resource `collab.mcp.ts` that piloted **composite `when` gates** — the collab +addon AND the per-sub-feature `getCollabFeatures()` flags (notes/polls/chat) +that the legacy registrar and resources checked at registration time — plus +the plugin-host swap and a 3-export `collab.bridge.ts` for the two remaining +legacy consumers, `tripService`'s trip summary and `mcp/tools/trips.ts`); +vacay followed (the largest MCP port yet — a 26-tool + 3-resource +`vacay.mcp.ts`, including the first fixed-URI `@Resource` behind a `when` +addon gate — plus the plugin-host swap and a 1-export `vacay.bridge.ts` for +`tripService`'s trip-window entry shift; the DTO ratchet for its 13 +grandfathered body contracts landed as a sibling commit); reservations +followed (the residue fold: the wrapper `ReservationsService` was already +DI-native at the edge, so the 626-line legacy module folded into it — a 5-tool ++ 1-resource `reservations.mcp.ts`, the plugin host's last plain-function +reservation import (`notifyBookingChange`) swapped for the injected service, +`TripsService` and `BookingImportService` converted from function imports to +injection, and a 9-export + 3-type `reservations.bridge.ts` for the legacy +`tripService`, the airtrail import/sync pair and the still-legacy transports +registrar (the transit registrar has since migrated to `transit.mcp.ts`); the +DTO ratchet for its 4 grandfathered body contracts +landed as a sibling commit, which also loosened the shared positions schema to +the real wire contract — `day_plan_position` optional, pinned by RESV-006); +day followed (the 592-line legacy `dayService` folded into the existing +wrapper `DaysService` — including the accommodation SQL that +`nest/reservations/accommodations.service.ts` now reaches via an injected +`DaysService` — with the hand-rolled `BEGIN`/`COMMIT` blocks in +reorder/insert converted to `db.transaction()`; a 7-tool + 2-resource +`days.mcp.ts`, the plugin host's 11-symbol import swapped for the injected +service, `TripsService` + the assignments/reservations MCP controllers +converted to injection, and a 6-export `days.bridge.ts` for the legacy +`tripService` and the still-legacy transports registrar (transit has since +migrated); the DTO +ratchet for its 4 day + 2 accommodation grandfathered body contracts landed +as a sibling commit); +permissions followed (the first Wave-2 **cross-cutting** migration and the +first greenfield module in the series — no prior wrapper, controller, MCP +surface or DTO of its own: a new `nest/permissions/` whose +`PermissionsService` is injected by 16 domain services, the airtrail-import +controller and `PluginHostDepsFactory` (its 16th constructor dep) in one move, +plus a 5-function `permissions.bridge.ts` for `mcp/tools/_shared.ts` — one +repoint covering every `hasTripPermission` call site — and the four legacy +consumers adminService/authService/backupService/collectionsService; the +permissions **cache stays module-scoped** in `permissions.service.ts` on +purpose, so the bridge instance and the container singleton share one cache +and backup-restore's bridge-side `invalidatePermissionsCache()` flushes what +request handlers serve; the domain e2e suites swapped their path mocks for a +`vi.spyOn(app.get(PermissionsService), 'checkPermission')` instance spy); +auditLog followed (the other Wave-2 half, split into five files: the +injectable `AuditService` (`writeAudit` over `DatabaseService`), the pure +`client-ip.ts` (files.constants precedent — the four getClientIp-only +controllers stay plain imports), the deliberately side-effectful plain +`audit-log.logger.ts` — frozen-at-import `LOG_LEVEL` and the import-time +`data/logs` mkdir are a documented parity exception to the no-side-effects +rule because `index.ts` lazy-requires it pre-container and tests/setup.ts +sets the level pre-import — plus `audit.module.ts` and a full-surface +`audit.bridge.ts` for `mcp/index.ts`, `mcp/oauthProvider.ts` and the legacy +airtrail/immich/oauth services, while log*-only consumers (index.ts's lazy +require strings, scheduler, globalMiddleware, notifications) point at the +logger directly; 8 controllers + `PluginRuntimeService` inject `AuditService`, +and the domain e2e suites dropped the audit mock entirely — writeAudit runs +for real against an `audit_log` table in their temp DBs). +The exchangeRateService fold followed (the pure-infra FX helper — Frankfurter +fetch + module-scoped 6h cache, no SQL, no controller, no MCP registrar of its +own — folded into `nest/budget/` as a dep-free `ExchangeRatesService`, injected +by `BudgetService` and `PluginHostDepsFactory` (its 17th constructor dep); the +rate **cache stays module-scoped** like the permissions cache, so any +out-of-container instance and the container singleton share one cached +upstream feed). +budgetService followed (the 755-line Wave-4 money core folded into the wrapper +`BudgetService`: items/members/payers CRUD, the FX freeze + rebase paths and +the settlement maths with `splitEqualShares` gone private; the freeze-then-write +composites kept their wrapper names while the raw settlement writes became +`insertSettlement`/`applySettlementUpdate` so the MCP paths keep skipping the +freeze; the 11-tool `mcp/tools/budget.ts` registrar + 3 `resources.ts` budget +resources moved to `budget.mcp.ts`; TripsService, ReservationsService (+ its +MCP class) and BookingImportService inject `BudgetService`; a 4-export +`budget.bridge.ts` served the legacy tripService/userCleanupService and the +trips/transports registrars (deleted 2026-08-11: BudgetMcp's demo guard moved +off AuthService onto `isDemoUserId`, BudgetModule dropped AuthModule, and +`UserCleanupService` injects `BudgetService` now); +`exchange-rates.bridge.ts` was deleted with its +last consumers, and the controller adopted `budget.dto.ts` — all nine +allow-list entries removed). +tripService followed (the 1121-line Wave-4 hub — the biggest fold — moved into +the wrapper `TripsService`: TRIP_SELECT + list/create/get, the +`generateDays` two-phase renumber engine, the updateTrip date-shift +transaction, the member/guest lifecycle (#973/#1362), the ICS export with its +module-scoped tz-validity cache, `copyTripById` and `getTripSummary`; its six +bridge imports became injected services (CollabService + VacayService joined +the constructor); the 10-tool trips registrar + 3 `resources.ts` trip +resources + the trip-summary prompt moved to `trips.mcp.ts` — the first +`@Prompt` use, with the fire-once static-token deprecation notice now riding +the `registry.attach` ctx — and the 3 share-link tools it carried moved to +`share.mcp.ts` on the `canShareTrips` predicate (delete_trip and the +canReadTrips reads are predicates too — the broadened legacy gates have no +declarative equivalent); FeedsService and the plugin host inject +`TripsService` (its 20th constructor dep); a 4-export `trips.bridge.ts` +serves budget.mcp.ts / packing.mcp.ts / costs.rpc.ts (verified-permanent +2026-08-11: the trip read-model/members modules import the budget and packing +domains, so injecting any backing service there closes a real cycle; the +prompts registrar it also served is long migrated); +`todo.bridge.ts`, `share.bridge.ts`, `collab.bridge.ts` and `vacay.bridge.ts` +were deleted with their last consumers and the unused days/budget bridge +exports pruned; the controller adopted `trips.dto.ts` — all seven allow-list +entries removed). +mapsService followed (the 1429-line geo core — Google Places, Nominatim, +Overpass mirror racing, Wikimedia photos, Maps-URL resolution — folded into the +wrapper `MapsService`; the pure parser/UA/POI-category helpers moved to +`maps.helpers.ts` as plain exports (files.constants/client-ip precedent — +the DI-native TransitService's User-Agent imports from there, not a bridge), the module-scoped +POI cache / photo-fetch semaphore / frozen Overpass mirrors stayed module-scoped +on purpose (permissions-cache precedent: any out-of-container instance and the +DI singleton share them); the 3 geo tools left the mixed `mcp/tools/mapsWeather.ts` +registrar for the decorator-driven `maps.mcp.ts` (the registrar file survives — +its weather + airport tools await their own migrations); BookingImportService +injects `MapsService` for its Nominatim geocoding, and a 3-export +`maps.bridge.ts` served the legacy placeEnrichment helper and the places +registrar — both absorbed by the 2026-07 place fold, which deleted the bridge). +transitService followed (the first fully SQL-free domain fold — the 333-line +Transitous/MOTIS proxy became a dep-free `TransitService` (no +`DatabaseService`; the ExchangeRatesService precedent), its response cache, +frozen-at-import `TRANSIT_API_BASE` and lazy User-Agent memo staying +module-scoped on purpose; the pure `deriveTransitStats` + mode whitelist + +itinerary types moved to `transit.helpers.ts` (maps.helpers precedent) so the +downstream legacy `transitItineraryService` needed no bridge (since relocated +into the domain as `transit-itinerary.helpers.ts`); the whole 3-tool +`mcp/tools/transit.ts` registrar moved to `transit.mcp.ts` — the two geo +search tools on `access: { group: 'geo', mode: 'read' }` and +`create_transit_journey` on `reservations:write`, with its days/reservations +bridge imports becoming injected `DaysService`/`ReservationsService` (+ +`DatabaseService` for `canAccessTrip`) — leaving **zero bridge files** and the +transports registrar as `days.bridge.ts`'s last consumer). +placeService followed (the step-4 tail: the 1029-line place core — the +CRUD + ratings SQL, the GPX/KML/KMZ importers and the Google/Naver list +importers — folded into the wrapper `PlacesService`, with the pure pieces +(frozen XML parsers, the KMZ unpacker, the dedup predicates, the Google +hex-id parsers, `reclaimPhotoCache`) moving to `places.helpers.ts` on the +maps.helpers precedent; the 10-tool `mcp/tools/places.ts` registrar + the +`trek://trips/{tripId}/places` resource moved to `places.mcp.ts` — +`search_place` came along because its gate is `places:read`, not the geo +group, and now injects `MapsService`; TripsService, DaysMcp, +BookingImportService and the plugin host (its 21st constructor dep) inject +`PlacesService`, leaving **zero bridge files** for the domain; the two +`assignments.bridge` imports stay in `places.mcp.ts` on purpose — +AssignmentsModule imports DaysModule and DaysModule now imports +PlacesModule, so injecting would close a +DaysModule → PlacesModule → AssignmentsModule → DaysModule cycle +(reservations.mcp.ts uses the same seam for the same reason). The sibling +`placeEnrichment` fold went further than the recipe's minimum: the 168-line +helper's DB/websocket/Maps half became `PlacesService` methods over the +injected `DatabaseService`/`RealtimeService`/`MapsService` and its pure +match selector joined `places.helpers.ts`, which retired **`maps.bridge.ts`** +with its last consumer. The DTO ratchet for its 7 grandfathered body +contracts landed as a third commit, which also loosened +`placeBulkUpdateRequestSchema.ids` (`.min(1)` dropped) so the endpoint's +empty-list short-circuit stays reachable, and retired three bespoke 400 +strings — 'Place name is required', 'ids must be an array of numbers' and +'URL is required' — in favour of the pipe envelope, accepting that a +malformed body now 400s ahead of the trip-access 404 (the todo/trips trade). +transitItineraryService followed (the first pure-helpers relocation with no +service fold at all — the 287-line module is 100% pure, so the recipe's SQL / +bridge / DTO / plugin-host steps were all no-ops: the Zod itinerary schemas + +endpoint/metadata builders moved byte-identical to +`transit-itinerary.helpers.ts`, next to `transit.helpers.ts`; the schemas +**must** stay module-level plain exports because `transit.mcp.ts` consumes +them inside `@Tool({ inputSchema })` decorators, which evaluate at module load +before any container exists; the sole consumer — the in-container +`transit.mcp.ts` — was a one-import repoint, closing step 4 of the +dependency-honest order; the legacy module had no direct suite, so a new +21-case `TRANSIT-ITIN-*` characterization suite now pins all 12 superRefine +error strings, the `??` time fallbacks, the coordinate tolerances and the +reservation endpoint/metadata builder). +collectionsService followed (the biggest single fold yet — 1024 lines / 28 +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` — 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 +didn't relocate verbatim). +atlasService followed (1612 lines split two ways, places precedent: the DB +half — stats aggregation, visited countries/regions with the #1490 +tombstone/cascade logic, bucket-list CRUD — folded into `AtlasService` over +DatabaseService alone, while the ~750-line pure-geo half — the bundled +admin0/admin1 stores and their #1576 OOM-shaped streaming builders, the +point-in-polygon indexes, Nominatim geocoding with its shared ≥1.1s throttle, +the 50k geocode cache with its import-time unref'd cleanup interval, and the +`geocodingInFlight` dedup set — moved to the plain module `atlas-geo.ts` so +those caches stay process-global across the container instance, the bridge +instance and test helpers; `assetPath` re-anchored one directory deeper, the +only non-verbatim line. The 10-tool legacy registrar `mcp/tools/atlas.ts` +plus all four atlas resources in `mcp/resources.ts` (`trek://bucket-list`, +`trek://visited-countries`, `trek://atlas/stats`, `trek://atlas/regions`) +moved to `atlas.mcp.ts` — here the `when:` atlas-addon gate IS parity, since +the legacy registrar and resources both gated on the addon while the REST +controller deliberately does not; the mark_region_visited / +get_country_atlas_places no-uppercase divergence from REST relocated +untouched. The plugin host swapped its 9 atlas imports for the injected +service — its 23rd constructor dep — and a minimal 2-export `atlas.bridge.ts` +existed solely for the legacy `authService.getTravelStats` edge +(`getCountryFromCoords` re-exported straight from atlas-geo, +`getHiddenCountries` over the bridge instance); it died with the auth fold, as +predicted. resources.test.ts retired with its last two cases — every resource +it covered now lives in the domain suites. The trailing `fix(server)` quirk +commit then wrapped the four multi-statement mark/unmark writes in +`db.transaction()` (the region cascade nests as a savepoint), made the +trip-less `countryPlaces` early return honour `manually_marked`, fixed the +`|| null` bindings that dropped `lat: 0`/`lng: 0`/`notes: ''` on bucket +update, user-scoped the mutating bucket SQL, and uppercased the MCP +region/country-places codes to match REST — each pinned by +`ATLAS-SVC-031…036` plus two MCP casing cases; see migration-graph.md's +"Quirks fixed after the atlas fold".) +authService followed (1497 lines, the biggest fold and the head of the +auth → admin → oauth chain: the pure crypto half — backup-code +hash/match/generate, `stripUserForClient`, key masking, the import-time +`DUMMY_PASSWORD_HASH` timing equaliser and the `avatarDir` mkdir (both +documented parity exceptions) — moved to the plain module `auth.helpers.ts`, +while the DB half — toggles/app-config, register/login with the CWE-208 +dummy-hash path, MFA setup/enable/disable/verify, the password-reset +throttle+token flows with their two `db.transaction()` revocation blocks, +API keys/settings, travel stats, MCP/ws/resource tokens, `isDemoUser` and +the two token verifiers — folded into `AuthService` over DatabaseService + +injected PermissionsService (ex `permissions.bridge`) and AtlasService (ex +`atlas.bridge`, which died on schedule). The `mfaSetupPending` and +per-email reset-throttle maps stay module-scoped (with the unref'd cleanup +interval) so the bridge instance and the container singleton share them; the +otplib `window: 1` mutation runs at module top; `require('../../../package.json')` +and `avatarDir` re-anchored one directory deeper — the only non-verbatim +lines besides the two injection swaps. No MCP registrar existed and the +plugin host never imported authService, so neither surface changed; the 15 +in-container `*.mcp.ts` demo guards now inject AuthService (their modules +import AuthModule) — except `atlas.mcp.ts`, which keeps a bridge import +because AuthService injects AtlasService and the reverse module edge would +close a cycle (places.mcp precedent). The 8-export `auth.bridge.ts` serves +`mcp/index.ts` token verification, the three legacy tool registrars' +`isDemoUser`, and the un-migrated adminService. +Tests moved with IDs preserved: authService.test.ts → auth.helpers.test.ts, +authServiceDb.test.ts → auth.service.test.ts (+ AUTH-BR-001…007 bridge +delegation); auth.e2e converted DI-native — the 30-method whole-module mock +died, login now runs real bcrypt and real audit rows; oidc.e2e switched its +dead path-mock for a `vi.spyOn(app.get(AuthService), …)` instance stub.) +oidcService followed (the frontier cash-in the auth fold predicted: a pure +fold of the 508-line module into the existing wrapper `OidcService` over +DatabaseService + injected AuthService — the `resolveAuthToggles` +`auth.bridge` import became the injection, the first repoint-consumer of that +bridge to migrate. No MCP registrar, no plugin-host import, and **no bridge +at all**: nothing outside the container consumes the domain (day-notes/ +trip-invite class), so the `pendingStates`/`authCodes` maps, their two sweep +`setInterval`s, the single-slot discovery cache and the JWKS cache became +**instance state** — the module-scope precedent (permissions/atlas-geo/auth +maps) applies only where a bridge instance must share state, and none exists +here; the sweepers start in the constructor and are cleared in +`onModuleDestroy`, the one wire-invisible deviation. The `uuid`/`bcryptjs` +lazy requires, the `invite_exhausted` reference-sentinel transaction (reshaped +one line for `DatabaseService.transaction`) and every SQL string relocated +verbatim; controller and its unit suite needed zero edits. Tests moved with +IDs preserved: oidcService.test.ts → oidc.service.test.ts (OIDC-SVC-001…045, +superseding the 18-case delegation-shim suite; 046–048 carry its wrapper +cases); oidc.e2e swapped its whole-module path mock for +`vi.spyOn(app.get(OidcService), …)` instance spies; the integration suite +spies the four HTTP methods on the container instance while driving the real +state maps on that same instance.) +passkeyService followed (the last frontier member of the auth stack: the +364-line WebAuthn module folded into a new `nest/auth/passkey.service.ts` +`PasskeyService` over DatabaseService + injected AuthService — its three +`auth.bridge` imports resolved to `this.auth.generateToken` plus plain +`stripUserForClient`/`avatarUrl` helper imports, and `resolveWebauthnConfig` +stayed a plain `services/webauthnConfig` import at the time (it has since +become the injected `WebauthnConfigService`, which also feeds auth.service's +`passkey_configured`). No MCP registrar, no plugin-host +import, and **no bridge at all** — both consumers were already in-container: +`PasskeyController` swapped its `import *` shim for the injection, and +`AdminService` swapped its `adminResetPasskeys` function import for the +injected service (`AuthModule` now exports PasskeyService and `AdminModule` +imports AuthModule — the todo→TripsService in-container precedent). No +module-level mutable state existed to preserve: the challenge store is +DB-backed (`webauthn_challenges`, single-use `DELETE … RETURNING` claim, +5-min TTL), so the fold is a plain stateless injectable — every SQL string, +error string and the counter/login-bookkeeping transaction relocated +verbatim. Tests: the legacy module had **no service-level suite**, so +PASSKEY-SVC-001…030 were written fresh with the fold (characterization over a +real `:memory:` DB, `@simplewebauthn/server` mocked at the ceremony-verdict +boundary — the repo's first such mock); auth-guard.test.ts's PasskeyController +block swapped its path mock for a constructor stub.) +notificationService followed (the notifications fan-in, step 3 of the +dependency-honest order: the `send()` dispatcher **and** the +`inAppNotifications.ts` store folded together into the existing +`NotificationsService` over DatabaseService + RealtimeService — the wrapper's +in-app delegation became the real SQL, while the prefs matrix, the smtp/ +webhook/ntfy transports, the channel registry and `inAppNotificationActions` +stay plain-module imports (graph-classified infra helpers, complete with +their registry⇄prefs cycle). A one-export `notifications.bridge.ts` (`send`) +covered the outside-container consumers — scheduler's two cron `require`s, +legacy adminService and memories/{unified,synology} (that bridge died +2026-08-10 with the scheduler migration; `notifications.instance.ts` carries +the surviving cycle-dodge bridges) — **and** the six +deliberately-lazy fire-and-forget `import().then(({ send }) => …)` sends in +migrated Nest services (collab/collections/packing/reservations/trips/vacay, +path-only repoints; kept lazy so a send can never block or cycle a domain +module). In-container static consumers inject instead: `AdminController`'s +dev test send (AdminModule → NotificationsModule) and the plugin host +(`NotificationsService` = `PluginHostDepsFactory`'s 24th dep). The 5-tool +legacy registrar + the `notifications-in-app` resource moved 1:1 to +`notifications.mcp.ts` (no `when:` — the domain is core, not addon-gated). +Tests moved with IDs preserved: notificationService.test.ts → +notifications.service.test.ts (NSVC-001…019/NTFY-SVCB-*/NSVC-PLUG-001…007, +plus the NSVC-020 bridge pin), inAppNotificationPrefs.test.ts → +notifications.inapp-prefs.test.ts (INOTIF-*); ~18 suites repointed their +path mocks/warm-ups to the bridge; admin.controller.test and the plugin-host +suite converted theirs to constructor stubs; the module e2e went DI-native +(real notifications DDL, prefs/transports still path-mocked).) +adminService followed (the last Wave-5 god file and the first fold where recipe +steps 2-4 were all no-ops or near-no-ops: **no `src/mcp/tools/admin.ts` has ever +existed** — the "11 MCP consumers" figure carried in `migrate.md` / +`migration-graph.md` predated the Phase-0 addons extraction, so no registrar +moved, `mcp/tools.ts` / `mcp/resources.ts` / `mcp-test-controllers.ts` were +untouched — and `plugin-host-deps.factory.ts` never imported the domain either +(its addon reads already went through the DI-native `AddonsService`). The +851-line module folded into the wrapper `AdminService` over `DatabaseService` +plus injected Settings/Addons/Passkey/Packing/Auth/Permissions/Notifications +services: the `auth.bridge` (`resolveAuthToggles`), `notifications.bridge` +(`send`) and `permissions.bridge` (`getAllPermissions`/`savePermissions`) +imports all became injections, while `PERMISSION_ACTIONS` stayed a plain const +import and the `mcp/sessionManager` deep import kept its anti-cycle comment. +The pure + module-scoped half moved to `admin.helpers.ts` — `compareVersions`, +`utcSuffix`, `BCRYPT_COST`, the import-time `isDocker` probe (a documented +parity exception, auth.helpers precedent) and the 5-minute version cache, which +stays module-scoped so the bridge instance and the container singleton share it. +Ahead of the fold the 11 packing-template functions relocated to +`PackingService`, which already owned all three template tables +(`saveAsTemplate` writes every one of them) — that also resolved the `admin-2` +residual without a bridge, since `packing.mcp.ts` already injects the service; +`AdminModule` gained PackingModule and PermissionsModule, both cycle-free. +A 1-export `admin.bridge.ts` (`checkAndNotifyVersion`) served the only +out-of-container consumer, `scheduler.ts`'s daily cron (both died 2026-08-10 +with the scheduler migration — `VersionCheckJob` injects `AdminService`). +Four lines are +non-verbatim, all path re-anchoring one directory deeper for `nest/admin/` +(`rotateJwtSecret`'s data dir, the `package.json` version require, the +websocket/demo-reset lazy requires) — both resolved paths verified against the +emitted `dist/` layout. Tests moved with IDs preserved (adminService.test.ts → +`nest/admin.service.test.ts`, ADMIN-SVC-001…069 incl. the pre-existing 029/030 +gap and duplicated 069, + ADMIN-BR-001 pinning the bridge and the shared version +cache; versionNotification.test.ts → `nest/admin.version-notification.test.ts`, +VNOTIF-001…007; the template cases rode along to `packing.service.test.ts`), and +the module e2e went DI-native — its 3-method whole-module mock died, 6 cases +became 15 over real SQL. A sibling DTO ratchet cleared all twenty +`AdminController` allow-list entries — the largest single block — trading the +three `'enabled must be a boolean'` checks plus `'permissions object required'` +and `'Object body required'` for the pipe envelope; the schemas are +deliberately permissive wherever the service owns a bespoke 400 of its own.) +Repeat these steps per +service (next up: **backupService** / **airtrail** — +per the dependency-honest order in +`migration-graph.md`). This is a +**pure relocation** — byte-identical +SQL, statuses, bodies, and error strings. The plugin RPC host is **no longer a +bridge consumer**: since Option A of `src/nest/plugins/DI-MIGRATION.md` it +injects domain services via `PluginHostDepsFactory`, so a migrated domain adds +`exports: [XService]` + a `PluginsModule` import instead of a bridge entry. +Only the pre-`app.init()` MCP/OAuth mount still needs bridges — the module- +cycle seams are folded, nothing on the websocket path imports one, and the +crons all inject now (`*.job.ts` providers on `scheduling/CronRegistrarService`). + +1. **Move the SQL** into `.service.ts` as methods over an injected + `DatabaseService` (`this.db.all/get/run/prepare`; multi-statement writes go + through `await this.uow.transactional(...)` instead — see the root `CLAUDE.md`; + strict constructor injection, no `@Optional()`). Preserve every quirk: falsy-coercion + defaults (`x || fallback`, never `??`), post-insert/post-update re-selects (no + RETURNING), `COALESCE` semantics. If a controller already wraps the legacy + functions, do not change the service's method surface. The module needs no + `imports: [DatabaseModule]` — it's `@Global`. +2. **Add `.bridge.ts`** next to the service **only if non-Nest consumers + exist** (legacy MCP tool registrars, websocket — the plugin RPC host and the + crons now inject instead, see above). It builds a module-level instance over + the shared connection Proxy — `new XService(new DatabaseService(db))`, + reinitialize-proof, same pattern as `nest/todo/todo.bridge.ts` — and exports + the legacy function names 1:1. + Container code injects the service; only outside-container code imports the + bridge. When porting an MCP registrar, note the `access: { group, mode }` + markers are typed against the scope-derived `ScopeGroup` union and + boot-validated by `trekMcpValidateAccess` (`src/mcp/nest-mcp-policy.ts`) — + an unknown group, or `mode: 'write'` on a read-only group (`geo`, + `weather`), fails app boot. **`mode` is typed the same way**: it is the mode + half of `Scope` (`read | write | delete | share`), fed into + `src/nest-mcp/types.ts` through `McpAccessModeRegistry` exactly like the + group registry. A scope that is neither read nor write — `journey:share` is the + one today — gets a real marker, *not* a `(ctx) => canX(ctx.scopes)` + predicate: predicates bypass the policy entirely and are invisible to the + boot gate, so a scope typo in one would ship as a silent full-access tool. + Reach for a predicate only when the gate genuinely is not a scope. *(Design decision, settled with the tags pilot: + MCP tools stay outside the container and use the bridge. The alternative — handing the Nest app to the + MCP layer via `app.get(XService)` — was rejected: it would thread the container + through `mcpHandler` + every tool registrar and force a Nest bootstrap into the + container-less `tests/helpers/mcp-harness.ts` used by ~25 MCP suites.)* +3. **Repoint non-Nest consumers** — import-path-only diffs from + `services/Service` to `nest//.bridge`; call sites unchanged. +4. **Delete the legacy service** once `grep -rn "services/Service" src tests` + only hits the tests you're rewriting. +5. **Tests:** + - Move `tests/unit/services/Service.test.ts` → + `tests/unit/nest/.service.test.ts`, preserving case IDs. Construct the + service directly — `new XService(new DatabaseService(testDb))` — no + TestingModule, no `overrideProvider` (repo convention). Add one delegation + case per bridge export: the bridge sits under `src/nest/**`, inside the ≥80% + coverage gate, and these cases pin it deterministically. + - Convert the module e2e to the DI-native pattern (exemplar + `tests/e2e/trips.e2e.test.ts`): temp-db DDL for the domain's tables, + `imports: [DatabaseModule, Module]`, real SQL assertions. Keep only + the `vi.mock('../../src/db/database', …)` — the auth guard still reads users + through the singleton, and `DatabaseModule`'s factory picks up the same + mocked db. Drop the legacy-service mock entirely. + - Suites that mocked the legacy module mock the bridge path instead — same + factory shape, path-only change. (For the plugin host suite, + `plugin-host-deps.factory.test.ts`, the domain becomes a constructor stub + instead of a path mock.) +6. **Verify** (from `server/`): `npm run typecheck`, `test:unit`, + `test:integration`, `test:e2e`, `lint:check`, `test:coverage`. diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 2429d229e..38e6e1404 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -1,988 +1,200 @@ -# NestJS migration layer — module & test guide +# Nest modules: blueprint and test guide -This folder holds the co-hosted NestJS app that incrementally strangles the legacy -Express API (see the "Brownfield Rewrite" board). Until a prefix is migrated, the -top-level dispatcher in `src/index.ts` routes it to the legacy app; migrated -prefixes go to Nest. **Weather (`weather/`) is the reference implementation** — copy -its shape when migrating a new domain. +Every request goes through Nest; Express is only the platform underneath +`@nestjs/platform-express`. Each domain is a DI module in this folder. +**Weather (`weather/`) is the reference implementation**: copy its shape for a +new domain. The rules every module follows (repositories, transactions, +config, outbound fetch, money) are in `server/CLAUDE.md`; this file is the +shape and the reasoning behind it. How the code got here, from the legacy +services layer and its bridges, is in `server/docs/nest-migration-history.md`. ## Module layout (per domain) ``` -shared/src//.schema.ts(.spec.ts) # Zod contract — single source of truth -server/src/nest//.service.ts # business logic (ported 1:1 from the Express service) -server/src/nest//.controller.ts # same routes/verbs/params/status codes as Express -server/src/nest//.module.ts # registered in app.module.ts +shared/src//.schema.ts # Zod contract, the single source of truth +server/src/nest//.service.ts # business logic, an injectable provider +server/src/nest//.controller.ts # REST routes, thin: validate, call the service, shape the reply +server/src/nest//.module.ts # registered in app.module.ts +server/src/nest//.mcp.ts # MCP tools/resources/prompts (@Tool, @Resource, @Prompt) +server/src/nest//.rpc.ts # plugin RPC methods (@PluginMethod), see plugins/host/rpc-kit/README.md +server/src/nest//.job.ts # a cron, registered on scheduling/CronRegistrarService +server/src/nest//.helpers.ts # pure functions the service and its tests share ``` -Register the module in `app.module.ts` and it is live. There is no routing gate to -flip any more: **every request goes through Nest**. The `strangler.ts` matcher and -its `DEFAULT_NEST_PREFIXES` / `NEST_PREFIXES` escape hatch are gone — Express only -remains as the platform underneath `@nestjs/platform-express`. +`weather/` has every piece: `weather.controller.ts`, `weather.service.ts`, +`weather.module.ts`, `weather.mcp.ts`, `weather.rpc.ts`, and `weather.impl.ts` +for the outbound client with its deadline and size cap. -## Migrated so far +- Register the module in `app.module.ts` and it is live. +- SQL lives in repositories under `src/db/repositories/`, injected into the + service; multi-statement writes go through `UnitOfWork.transactional(...)` + (`database/unit-of-work.ts`). Outside an HTTP request (MCP, RPC, WS, cron) + open a context with `withRequestContext()` (`database/request-context.ts`). +- A domain that outgrows one class is split into modules, never grown: trips is + trips, trip-members, trip-membership, trip-invite, trip-read-model and + calendar. +- The MCP tool and the REST route are adapters over the same service method and + the same permission check. A validation or permission change lands on both in + the same change. +- The legacy services layer under src/ is deleted, and ESLint refuses any + import of a `services/` path. There are no `*.bridge.ts` files either; do not + add one. -- **Phase 1 (leaf):** weather, airports, config (public), system-notices, maps, - categories, tags, notifications, atlas. -- **Phase 2 (trip sub-domains):** vacay (addon), packing, todo. -- **DI-native services (legacy `src/services/*` deleted):** tags, categories, - todo, packing, day-notes, trip-invite, assignments, share, settings, files, - collab, vacay, reservations, day, permissions, audit, budget, trip, maps, - transit, place, transit-itinerary, collections, atlas, auth, oidc, passkey, - notifications, admin, webauthn-config, user-cleanup, oauth, wiki, mailer, - notification transports, notification preferences, memories (immich, synology, - unified, photo-resolver, thumbnails, trek-photo cache), journey, journeyShare - — see the migration recipe below. -- **Foundation (BE-Phase 1, complete):** the eight stateless helpers moved to - `common/`; every trip-access check routes through a repository - (`services/tripAccess.ts` and the dead `middleware/tripAccess.ts` are gone; - `DatabaseService`, the facade BE-Phase 1 originally routed this through, was - itself retired by the later ORM migration — see the root/server `CLAUDE.md`); - `queryHelpers` and `tripMembership` are providers; every cron is a domain - `*.job.ts` provider on `scheduling/CronRegistrarService` (`src/scheduler.ts` - is deleted). -- **Plugin RPC surface (BE-7, complete):** all 113 wire methods are - `@PluginMethod` / `@PluginOpenMethod` handlers on `@PluginController()` providers - in their own domains (`/.rpc.ts`; the three that belong to no - domain live in `plugins/host/rpc/`), and all 18 host-to-plugin hook contracts are - `@PluginHook` declarations on `PluginHooks`. `PluginRpcHost` no longer holds a - handler: it builds the granted subset of the dispatch map from the registry, - dispatches, audits and maps errors. `PluginHostDepsFactory` — the 26-argument - wiring sheet named throughout the log below — is now - `PluginRpcHostFactory` and injects two things. See - `plugins/host/rpc-kit/README.md`. -- **Plugin module split (complete):** `PluginsModule` was 21 controllers, 24 domain - imports and 10 providers in one class, so `AdminModule` importing it for one - cascade-disable call inherited the whole graph. It is now `PluginsRuntimeModule` - (supervisor, capability router, hook contracts — the half with the domain imports), - `PluginContributionsModule` (the 14 read-only hook controllers, in - `plugins/contributions/`), `PluginOAuthModule` (`plugins/oauth/`) and a composition - root holding the install and delivery controllers. `AdminModule` imports the runtime - alone. Note the invariant the boot-time coverage check now depends on: importing - `PluginsRuntimeModule` must pull in EVERY domain that owns part of the wire surface, - which is why `WeatherModule` sits in its import list. -- **Guards (BE-Phase 2, done):** the JWT verify lives in `auth/`, and - `middleware/auth.ts`, `validate.ts`, `idempotency.ts` and `mfaPolicy.ts` are - deleted; the three addon guards are one `AddonGuard` + `@RequireAddon`; the - demo-mode block is one condition. `TripAccessGuard` and `TripOwnerGuard` cover - the trip-scoped and owner-only routes. +## Reaching the container from outside it - **Authentication is default-deny.** `GlobalAuthGuard` is an `APP_GUARD`: a route - is authenticated unless it carries `@Public(reason)` or `@OptionalAuth(reason)`, - or declares its own `@UseGuards` chain. A controller that declares a chain keeps - it — global guards run before route guards, so an addon-gated controller has to - stay in charge of answering 404 before anything answers 401, or the response - leaks which addons are installed. The global guard still *resolves* the user in - that case, without refusing, because `MfaPolicyGuard` runs behind it and reads - `req.user`. +Code that runs outside the container never builds a second instance of a +provider with `new`. `bootstrap.ts` resolves the singleton with `app.get(...)` +before `app.init()` and hands it in: the `httpConfig.KEY` namespace, the +`DatabaseLifecycle` that opens the connection, and the `StorageService` the +platform upload routes write through. - Adding a `@Public()` route is not a local decision: `validateRouteGuards` runs in - `buildApp()` and refuses to boot unless the route is also in - `PUBLIC_ROUTE_ALLOW_LIST` in `common/validate-route-guards.ts`. That list is the - whole anonymous surface of the server, in one reviewable place. Stale entries - fail too. +Express middleware that needs DI has two mounts: +`consumer.apply(...).forRoutes('')` in a module's `configure()` +(an Express prefix mount; `OauthModule` mounts the MCP SDK's authorize and +register routers that way), or, for middleware that must see the original +`req.url`, a pathless `app.use` in `bootstrap.ts` over a container-resolved +factory provider (`MCP_METADATA_MIDDLEWARE`). `mcp-transport/` (the `/mcp` +controller and service) and `mcp-shared/` (`McpToolGuardsService` for the +`*.mcp.ts` classes) are the module shapes for MCP. -**`src/services/` is gone.** It was the legacy layer this whole migration existed -to empty, and the last of it — `airtrail/`, 1305 lines of plain functions over the -better-sqlite3 singleton — folded into `integrations/`. An ESLint -`no-restricted-imports` rule now refuses any import of a `services/` path, because -the directory disappearing is not what keeps it gone: it grew one reasonable file -at a time. Backup made the same trip earlier: `services/backupService.ts` is -`nest/backup/backup.impl.ts`, imported only by its own domain. It is still a -module of free functions, but it no longer closes or reopens the core DB handle -itself: `BackupService` hands it the injected database backup port -(`DATABASE_BACKUP`, `SqliteDatabaseBackup`), which snapshots and swaps the file -through `DatabaseLifecycle`, the provider that owns the connection. The demo -reset goes through the same port. Another engine plugs in there, with a logical -dump in place of `VACUUM INTO` and the file swap. +A decorator's options are built while the class is defined, so an MCP `when:` +gate cannot reach `this`. `src/nest-mcp/` hands the gate its declaring +instance, and `addons/addon-gate.ts` turns that into `addonGate(ADDON_IDS.X)` +over an injected `AddonsService`. -Note that the trip-access and `canEdit` methods on the domain services are **not** -dead weight waiting for a guard: their callers are overwhelmingly the `*.mcp.ts` -tools, which never pass through an HTTP guard. In the five domains piloted for -`TripAccessGuard`, 40 of the 46 callers are MCP; only 6 sit in controllers. +## Cross-cutting pieces -## Cross-cutting Foundation pieces +- `common/`: the exception filter, the Zod validation pipe, the idempotency + interceptor (`X-Idempotency-Key` replay), `validate-route-guards.ts`, and + stateless helpers (`avatarUrl`, `conflictResult`, `demo`, `passwordPolicy`, + `timezoneService`, `cookie`, `rowShape`, `geo`, `crypto/`). The helpers are + free functions on purpose: numbered migrations and `src/demo/demo-seed.ts` + import them from outside the container. `geo.ts` keeps the clamped haversine, + since `asin` of a value a hair over 1 is NaN and a NaN distance fails every + comparison silently. +- `auth/jwt-verify.ts`: `extractToken` and `verifyJwtAndLoadUser`, the one + session check behind the guards, the MCP bearer path and the download token. + `JWT_SECRET` stays a live binding from `src/config.ts`, because the admin panel + rotates it at runtime and a `registerAs` token would freeze the boot value. +- `addons/addon.guard.ts` with `@RequireAddon(addonId, label)`: declare + `AddonGuard` before `JwtAuthGuard`, so a disabled addon answers 404 to an + anonymous caller instead of revealing itself with a 401. +- `trip-membership/` (`joinTripAsMember` and the id-level membership reads) is a + leaf module because auth, trip-invite, oidc and the plugin host all need it. + `query-helpers/` holds the batch loaders that keep list endpoints off N+1. +- `storage/`: the driver contract in `storage.types.ts` (opaque keys, no rename + or append), the local, S3 and mirror drivers in `drivers/` behind one contract + suite (`tests/unit/nest/storage/storage-driver.contract.ts`), the registry in + `storage-registry.service.ts`, and the admin surface. Domain services never + touch the filesystem or the S3 SDK. +- `memories/photo-provider.ts` and `providers/`: photo backends behind + `PhotoProviderRegistry.get(id)`. `PhotoAssetRef` exists because two services + took the same three ids in different orders. `local` is not a provider; both + dispatch sites branch on it. +- `realtime/`: `/ws` as a Nest gateway. `ws-state.ts` is module state on purpose + (test harnesses build `RealtimeService` outside the container). The socket id + stays a monotonic integer, because `broadcast` excludes the originator with + `Number(excludeSid)`. `trek-ws.adapter.ts` builds the ws server itself so + `verifyClient` keeps the origin check, and attaches the per-socket `error` + listener first. `buildApp()` binds the adapter to the http server before + `app.init()`. +- `geo/`: the one Nominatim client (`nominatimFetch` in `nominatim.client.ts`), + with the throttle cursor and the cache as module state, two lanes + (interactive ahead of background), a cache key that carries the query shape, + and the `AbortSignal.timeout` built after the throttle wait. + `setGeoThrottleInterval(ms)` is the test seam. +- `scheduling/`: `CronRegistrarService` is the only way to schedule. Never a + `@Cron` decorator; it bypasses the test-time gate and the lease. +- `app-config/`: the `@nestjs/config` binding. Inject a boot-stable namespace + through its `registerAs` token, or read runtime-toggled values through + `RuntimeEnvService` / `readEnv()`; never `process.env`. The classification is + in `src/app-config/README.md`. -- `common/idempotency.interceptor.ts` — global `APP_INTERCEPTOR` replaying the - client's `X-Idempotency-Key` on mutations. It replaced the `applyIdempotency` - middleware, which is deleted; this is the only implementation. -- `common/` — the stateless helpers (`avatarUrl`, `conflictResult`, `demo`, - `passwordPolicy`, `timezoneService`, `cookie`, `rowShape`, `geo`, `crypto/`). - Free functions, not providers: the numbered migrations in `db/migrations/` - (e.g. the plaintext-secret encryption steps) and `demo/demo-seed.ts` import - them from outside the container and could not inject one. `geo.ts` holds the two haversines; there - were three implementations of the metre variant, differing by one clamp, and - the clamped form is the one kept — `asin` of a value a hair over 1 is NaN, and - a NaN distance silently fails every comparison it feeds instead of throwing. - `maps.helpers.ts` re-exports `haversineMetres` rather than defining it, because - callers and a test import it from there. -- `auth/jwt-verify.ts` — `extractToken` + `verifyJwtAndLoadUser`, the canonical - session check behind the four guards, the MCP bearer path and the file-download - query token. Free functions for the same reason. - `JWT_SECRET` stays a live binding from `src/config` and must NOT move to - `app-config`: the admin panel rotates it at runtime, and a `registerAs` token - would freeze the boot value. -- `query-helpers/` — the batch loaders that keep list endpoints off N+1 queries. -- `trip-membership/` — `joinTripAsMember`. Its own module rather than folded into - auth or trip-invite, because both call it (as do oidc and the plugin host) and - either home would put `AuthModule` and `TripInviteModule` in a cycle. -- `addons/addon.guard.ts` + `@RequireAddon(addonId, label)` — the addon route - gate. Declare `AddonGuard` **before** `JwtAuthGuard`: a disabled addon has to - answer 404 to anonymous callers, so the addon check must beat the 401. -- `common/demo-write.ts` — `isDemoWriteBlocked`, the demo-mode upload block. A - function and not a guard on purpose; see the gotcha below. -- `storage/` — the storage abstraction and its admin surface. `storage.types.ts` - defines the driver contract (opaque keys, no rename/append — S3 constraints - baked in) plus the `ServedCategory` seam: `STORAGE_CATEGORIES` (shared, 8, - configurable) + the legacy `photos` directory that is served and backed up - but not configurable. `drivers/` (local, s3, mirror) all pass one contract - suite (`tests/unit/nest/storage/storage-driver.contract.ts`). - `storage-registry.service.ts` merges built-ins ← env (`place-photos-local`) - ← `app_settings` rows, validates semantics (references, no mirror nesting), - falls back to last-good/defaults at boot but NEVER on admin writes - (`preview()` runs the real build first), seeds once from - `data/storage-config.json`, and hot-swaps on `reload()`. - `storage.service.ts` is the facade every byte-path uses. - `storage-admin.{controller,service}.ts` (+ `storage-secrets.ts`, - `storage-probe.ts`) carry `api/admin/storage`: masked GET of the effective - world, full-document PUT (unmask → preview → encrypt → one transaction → - reload → audit), and per-target test probes with ephemeral drivers. Secrets - live `enc:v1:`-encrypted inside the `storage.backends` JSON (the implicit - key covers installs without an explicit `ENCRYPTION_KEY`). The client - presents mirrors as replicas-on-primary; the wire keeps explicit mirror - backends. `storage-jobs.service.ts` runs backfill and category-migration - jobs (one at a time, TTL'd statuses). -- `memories/photo-provider.ts` + `providers/` — the PhotoProvider seam (#584). - Dispatch to a photo backend was a `switch (photo.provider)` in three places - (`photo-resolver.service.ts` twice, `journey-public.controller.ts` once, where - anything that was not Immich got handed to Synology). It is - `PhotoProviderRegistry.get(id)` now, over adapters registered under the - `PHOTO_PROVIDERS` multi-provider token in `memories.module.ts` — adding a - backend is one registration. - - Adapters, not `implements` on the services: ImmichService and - SynologyService are also the album/browse surfaces, with their own argument - orders and return shapes. The mapping lives in the adapter. - - `PhotoAssetRef` exists because the two services took the same three values - in different orders — `(userId, assetId, ownerId)` against - `(userId, targetUserId, photoId)`. A transposition compiled cleanly and - served another user's photo. - - `local` is deliberately NOT a provider. It is storage on this disk, it has - no row in `photo_providers`, and its "asset info" comes from the DB row - rather than from a backend. Both dispatch sites branch on it explicitly. - - The rename `memories/` → `media/` was considered and dropped: it is 275 - files of import churn, and `scripts/coverage-thresholds.mjs` buckets on one - path segment, so merging `photos/` into it would collapse two ratchet - entries into one and drop the stricter of the two. -- `realtime/` — the /ws transport, as a Nest gateway. - - `realtime.gateway.ts` owns the connection lifecycle and the handshake, with - `TripsRepository`/`UsersRepository`/`AppSettingsRepository` and - `EphemeralTokenService` injected instead of imported (Plan 4 converted it - off the deleted `DatabaseService`). It replaced `setupWebSocket(server)`, - which index.ts kicked off with a dynamic import after listen(). - - `ws-state.ts` holds the socket registry, and that is **module state on - purpose**. The no-Nest test harnesses build `new RealtimeService()` - outside the container and 115 test files `vi.mock` the `src/websocket` - stub; rooms on a provider instance would leave their broadcasts going to - an empty map, with no error and no log. Same reasoning as the geo - throttle cursor. - - `trek-ws.adapter.ts` exists because the stock `WsAdapter` dispatches on - `{ event, data }` and every deployed client sends `{ type, tripId }`. It - also builds the ws server itself rather than delegating: the base's port-0 - branch uses `noServer: true` and performs the upgrade from its own registry, - which never consults `verifyClient`, so delegating would silently drop the - origin check. The per-socket `error` listener is attached FIRST, because an - unhandled one kills the process (#1576), and the flood guard counts every - inbound frame before parsing, because malformed frames are the ones worth - limiting. - - The socket id stays a **monotonic integer**. The client echoes it as - `X-Socket-Id` and `broadcast` excludes the originator with - `Number(excludeSid)`; a uuid is `NaN`, `NaN === NaN` is false, and every - client would receive its own writes back. It shows up as drag-and-drop that - jumps, not as an error. - - `RealtimeGatewayModule` is imported by AppModule alone. RealtimeModule is - @Global, so a gateway there follows into every hand-assembled TestingModule, - and Nest's SocketModule then loads an adapter none of them set: it reaches - for `@nestjs/platform-socket.io` and calls `process.exit(1)` when it is - absent. - - `buildApp()` creates the http server and binds the adapter to it BEFORE - `app.init()`, and callers take it from `getHttpServer()`. Nest binds gateways - during init, so an adapter registered later binds nothing, and one given the - app instead of the server binds to Nest's internal server, which this process - never listens on. Either way the boot is green and no browser can connect. - - `src/websocket.ts` survives as a re-export. 115 test files `vi.mock` that - path to assert on broadcasts; repointing them all would be a large diff that - proves nothing. +## Guards -- `geo/` — the ONE Nominatim client (#576). There used to be two: atlas - throttled to the published 1.1s and cached its answers, maps did neither, so - five interactive routes fired straight at the service while the atlas loops - politely waited and OSM saw a single instance ignoring its rate limit. Both - domains now go through `nominatimFetch`. - - The throttle cursor and the cache are **module state, not provider state**, - for the permissions-cache reason: the no-Nest test harnesses build their - collaborators with `new`, outside the container, and a second copy of the - cursor is the very bug the fold removes. `GeoModule` is registered straight in AppModule and neither - `MapsModule` nor `AtlasModule` imports it — an import would claim an - injection edge that does not exist. - - `GeocodingService` owns exactly one thing: the cache-cleanup timer's - lifecycle, via `OnModuleInit`/`OnModuleDestroy`. Atlas used to start that - interval as an import-time side effect, documented as a parity exception. It - is not one any more. - - Two lanes over one cursor. Interactive calls (`/api/maps/search`, - `/autocomplete`, `/details`, `/reverse`, `/resolve-url`) take the next slot; - background calls (the atlas enrichment loops, which walk every uncached - place) yield to anything queued on a request path. - - The cache key carries the **query shape**, not coordinates alone. Atlas asks - at zoom 3 and zoom 8, maps at zoom 18; a coordinate-only key answered a - street lookup with a country. - - `AbortSignal.timeout` is built **after** the throttle wait. Built up front, - the 1.1s spent queueing would eat nearly half of the identity lookup's 2.5s - budget before the socket opens. - - `setGeoThrottleInterval(ms)` is a test seam, and `tests/setup.ts` zeroes it. - Nineteen maps cases drive these paths back to back with real timers; at the - real interval they slept nineteen seconds and covered nothing extra. A - function rather than a `NODE_ENV` check, for the reason app-config exists. +Authentication is default-deny: `GlobalAuthGuard` is an `APP_GUARD`, and a +route is authenticated unless it carries `@Public(reason)` or +`@OptionalAuth(reason)`, or declares its own `@UseGuards` chain. Adding a +`@Public()` route is not a local decision: `validateRouteGuards` refuses to +boot unless the route is also in `PUBLIC_ROUTE_ALLOW_LIST` +(`common/validate-route-guards.ts`), and stale entries fail too. -### Reaching the container from outside it +`permissions/trip-access.guard.ts` resolves `:tripId` once, answers 404 +"Trip not found" for anything the user cannot reach (never 403, which would +confirm the id exists) and hands the row to the handler through `@Trip()`; +`@RequirePermission` carries the action string. Two limits: -Two shapes, and the choice is settled: +- A guard runs before the body pipe, so a route whose validation must answer + 400 ahead of the trip 404 keeps an in-handler check (places' create, update + and import routes). +- A guard runs before interceptors, so a multipart upload checks in the handler: + a 404 sent while the client still streams resets the socket (ECONNRESET). -**Prefer handing the dependency in.** `bootstrap.ts` hands the `/mcp` handler -its registry via `setMcpRegistry(app.get(...))` after `buildApp()` — the -outside code names the capability it needs, structurally, never the provider -class, so no module cycle can close through it. (The retired `src/scheduler.ts` -was the fullest example — a structural `SchedulerDeps` interface filled by -`index.ts` — until every cron became an in-container `*.job.ts` provider and -the seam disappeared entirely, which is the real endgame for this shape.) - -**`.bridge.ts` is a retired shape** — zero bridge files remain since -the MCP/OAuth mount moved behind the container (2026-08-11). It served code -that ran before `app.init()` and so could not inject; its cost was real: a -bridge rebuilt its services with `new`, so adding one constructor parameter to -a service meant hand-editing every bridge that constructed it — five of them in -one PR, at the worst. If a pre-init consumer ever reappears, prefer the -`bootstrap.ts` pattern instead: resolve the container singleton with -`app.get(...)` before init (the `httpConfig.KEY` / discovery-metadata -precedent) rather than minting a second instance. - -**Express middleware with DI has two honest mounts** (first used by the mount -migration): `consumer.apply(...).forRoutes('')` in a module's -`configure()` — an Express prefix mount, exactly the legacy `app.use('/path')` -semantics, which is how `OauthModule` mounts the MCP SDK's authorize/register -routers and `PlatformModule` the consent-COOP override — and, for middleware -that must see the ORIGINAL `req.url` (a wildcard `forRoutes()` is a pattern -mount that strips the matched prefix), a pathless `app.use` in `bootstrap.ts` -over a container-resolved factory provider (`MCP_METADATA_MIDDLEWARE`). -`nest/mcp-transport/` (the /mcp controller + service over injected services) -and `nest/mcp-shared/` (`McpToolGuardsService` for the `*.mcp.ts` classes) are -the module shapes that replaced the last bridges. - -**A bridge imported from INSIDE the container hides an edge from the module -graph**, which is a different problem from serving a genuine outside consumer. -Nine `*.mcp.ts` files did that with `addons.bridge` for one boolean each, and -not by preference: a decorator's options object is built while the class is -being defined, so the `when:` closure had no `this` to reach an injected service -through. `src/nest-mcp/` hands the gate its declaring instance now, and -`addons/addon-gate.ts` turns that into `addonGate(ADDON_IDS.X)` over an injected -`AddonsService`. `reservations.mcp.ts` (assignments) and `atlas.mcp.ts` / -`journey.mcp.ts` (auth) went the same way. - -No in-container bridge uses are left. The last four folded 2026-08-11, and -their folds are the playbook for any edge that reappears — plain injection -would have closed a real cycle in every case, so each cut the knot with a -finer-grained module instead: - -- `budget.mcp.ts` / `packing.mcp.ts` / `costs.rpc.ts` → `trips.bridge`: - everything that owns the hydrated trip reads (TripsService, - TripReadModelService, TripMembersService) lives in modules importing the - budget/packing domains, so the consumers moved instead — the id-level - membership reads went to the dependency-free `TripMembershipModule` - (`getOwnerId` / `listMemberUserIds` / `listAccessibleTripIds`) and the two - cross-domain prompts moved above the read model onto - `trips/trip-prompts.mcp.ts`. `trips.bridge` (102 lines, ~25 hand-built - services) is deleted. -- `places.mcp.ts` → `assignments.bridge`: the cycle `DaysModule → - PlacesModule → AssignmentsModule → DaysModule` is real, but only - AssignmentsMcp needs DaysService — the service itself does not, so it moved - to a leaf `AssignmentsDomainModule` (the journey-domain split) that - PlacesModule imports. `assignments.bridge` is deleted. -- `reservations.controller.ts` → `airtrail.bridge`: the pull genuinely needs - ReservationsService, so `AirtrailModule` keeps it — but the write-back push - only needs the hydrated reservation READ. That read became - `ReservationsReadService` (leaf, the trek_photos pattern), the link - lifecycle + push became `AirtrailLinkService` in `AirtrailCoreModule`, and - the controller injects it. `airtrail.bridge` is deleted, and - `notifications.instance.ts` — whose last consumer it was — died with it. - -Two more were on this list and folded 2026-08-11: - -- `auth/user-cleanup.service.ts` → `budget.bridge` died when `BudgetModule` - dropped its `AuthModule` import (BudgetMcp's demo guard reads - `RuntimeEnvService` + the users table via `isDemoUserId`, not - `AuthService`), which let `AuthModule` import `BudgetModule` and - `UserCleanupService` inject `BudgetService`. `budget.bridge` is deleted. -- `backup/backup.impl.ts` → `permissions.bridge`: the restore path is free - functions, not a provider, so there is nothing for an injected service to - hang off — but the thing it needs is the cache, not the service. The cache - moved to `permissions/permissions-cache.ts` (the `oauth.pending-codes.ts` - precedent) and `backup.impl` imports the plain `invalidatePermissionsCache` - from there. - -`files.bridge` used to be on the list and is gone, resolved 2026-08-10 exactly -as predicted: `MulterModule.registerAsync` over an `AllowedFileTypesService` -leaf, and `files.controller.ts` / `journey.controller.ts` build their multer -options from that. `systemNotices/conditions.ts` similarly stopped importing -`addons.bridge`: the enablement check is threaded in through -`ConditionContext.addonEnabled` by the DI-native `SystemNoticesService`. - -`days`, `packing`, `photos` and `reservations` had no production consumer left -and are deleted. Each still built a service outside the container at module load -for nobody, and three domain docblocks were still sending readers to them. The -cases that exercised them kept their assertions and point at the service. -`journey.bridge` died 2026-08-11 with `src/mcp/resources.ts`, when the last -four legacy resources became `@Resource`/`@ResourceTemplate` methods on -`journey.mcp.ts`. - -Five bridges remain, all pinned by the one pre-`app.init()` MCP/OAuth mount -(`addons`, `audit`, `auth`, `oauth`, `permissions` — shrunk to the exports that -seam consumes) and all dying together if that mount ever moves behind the -container. When one loses its last outside-container consumer, delete it in -the same change rather than leaving it as a courtesy: an unused bridge reads -as a supported entry point. - -The nine fire-and-forget notification senders inject too. They were lazy -`import('../notifications/notifications.bridge').then(({ send }) => …)` calls -working around a cycle that no longer exists, and the laziness hid the edge -while handing each send a NotificationsService built outside the container. - -That sweep needed a method, because `tests/` sits outside `tsconfig`'s `include`: -a missed constructor argument does not fail to compile, it arrives as -`undefined`. Adding `tests` to a throwaway `tsconfig` and diffing the errors -before and after named all **46** hand-wired `new` sites exactly; a grep would -have been a guess. Worth repeating for any change that widens a service -constructor — and worth knowing that such a config reports **288 pre-existing -errors** in the suites, almost all deliberate partial constructions in the -harnesses (`new AuthService(db, permissions, atlas)` where the class takes six). -That is why `tests/` is not in the build's `include`, and cleaning it up is its -own piece of work. - -`notifications.instance.ts` held the out-of-container `NotificationsService` -for the cycle-dodge bridges and died 2026-08-11 with the last of them -(`airtrail.bridge`) — every production consumer injects the container -singleton now. The module-scoped channel registry it leaned on stays (the -no-Nest test harnesses still construct instances; CHOVR-015 pins the -sharing). -- `app-config/` — the `@nestjs/config` binding (`AppConfigModule`, global). Never - read `process.env` in a module (ESLint enforces this): inject a boot-stable - namespace via its `registerAs` token (`@Inject(storageConfig.KEY) … ConfigType<…>`) - or read runtime-toggled values live through `RuntimeEnvService` / `readEnv()` - from `src/app-config`. Every variable has exactly one owner, a token or - `readEnv()`, and `config-ownership.test.ts` fails on a variable read through - both. A provider that reads a data path takes `DataPathsService` - (`AdminService`, `StorageRegistryService`, and through it the storage - jobs); plain helper modules call `resolveDataPaths()` (see - `src/app-config/README.md` for which and why). The classification and - invariants live in `src/app-config/README.md`. - -## Parity gotchas worth remembering - -- A POST that answers with `res.json` in Express stays **200**; add `@HttpCode(200)` - (Nest defaults POST to 201). Creates that Express sends as 201 need nothing. -- Static sub-routes that collide with a `:id` param (e.g. `/in-app/all` vs - `/in-app/:id`, `/reorder` vs `/:id`) must be declared **before** the param route. -- Reproduce bespoke admin/error wording exactly — e.g. notifications' `test-smtp` - returns `{ error: 'Admin only' }`, not the AdminGuard's `Admin access required`. -- Trip-scoped routes verify trip access (404) and the relevant permission (403) - per handler and forward `X-Socket-Id` to the WebSocket broadcast. -- **Never gate a multipart upload with a guard.** Guards run before the parser, - so throwing from one leaves the request body unread and Node resets the - connection — the client sees `ECONNRESET`, not your 403. Check inside the - handler, after the interceptor has drained the upload. `PROFILE-015` is the - regression test. -- **`@Global()` only applies to modules that are in the graph.** An e2e - `TestingModule` built around one domain module does not get `AppConfigModule` - for free just because it is global: a provider injecting `RuntimeEnvService` - fails to resolve and takes every suite importing that module down with it. - Import it explicitly, the way `PermissionsModule` is handled. -- **A module-scoped registry that outside code writes into must stay a module, - not a provider.** The notification channel registry is the live example: - `PluginRuntimeService` pushes its channel getter in at `onModuleInit`, and - `notifications.instance.ts` builds its own `NotificationsService` for the - surviving cycle-dodge bridges. As a provider that instance would get a second, - empty registry and every plugin channel would go quiet — with no error - anywhere. `CHOVR-015` is the regression test. Same reasoning as - `oauth/oauth.pending-codes.ts`. -- **Self-registration by side-effect import is a trap.** The built-in channels - used to register because `notificationPreferencesService` happened to `import - './notifications/builtins'`. Dropping that one line would have silenced email, - webhook and ntfy without a failure. They are built from injected transports and - registered in `NotificationsService`'s constructor now, so every path that can - dispatch has them. -- **The coverage gate reads `src/nest/**` and nothing else.** Moving code in from - `src/mcp/` or `src/services/` does not just relocate it — it starts being - measured. PR #1844 landed `airports.mcp.ts` at 0% branches because - `search_airports`/`get_airport` had never had a test in either world, and the - aggregate fell to 79.86% with every suite green. Before pushing anything that - moves files into this tree, run `npm run test:coverage`, not just `npm test`. -- A guard with a constructor dependency has to be a registered provider - everywhere it is used. The three auth guards are dependency-free on purpose — - 38 directories apply them and 21 do not import `AuthModule`, so giving them a - dependency is its own migration, not a side effect of another change. +`permissions/trip-owner.guard.ts` (`@RequireTripOwner(message)`) demands +ownership, `trip.user_id === user.id`, and deliberately does not go through +`PermissionsService`, which says yes to every admin. The services keep +`verifyTripAccess`/`canEdit` anyway: most callers are `*.mcp.ts` tools, which +never pass an HTTP guard. ## Parity is law -A migrated route must be **byte-identical** for the client: same URL, method, -query/body, HTTP status, `Set-Cookie`, and JSON body — including bespoke error -strings. Where the legacy route returns a hand-written error (e.g. weather's -`{ error: 'Latitude and longitude are required' }`), reproduce that exact body in -the controller rather than relying on the generic `ZodValidationPipe` envelope. +A route is byte-identical for the client: same URL, method, query/body, HTTP +status, `Set-Cookie` and JSON body, bespoke error strings included. Where a +route answers with a hand-written error (weather's +`{ error: 'Latitude and longitude are required' }`), the controller reproduces +it instead of leaning on the generic pipe envelope. -## TripAccessGuard and TripOwnerGuard, and where they do not fit +- Nest defaults POST to 201; add `@HttpCode(200)` where the contract says 200. +- Declare static sub-routes (`/reorder`, `/in-app/all`) before `:id` routes. +- Reproduce bespoke wording exactly: notifications' `test-smtp` answers + `{ error: 'Admin only' }`, not the AdminGuard's `Admin access required`. +- Trip-scoped routes forward `X-Socket-Id` to the broadcast. +- `@Global()` only reaches modules in the graph: a `TestingModule` built around + one domain imports `AppConfigModule` explicitly if a provider needs + `RuntimeEnvService`. +- A module-scoped registry that outside code writes into stays a module, not a + provider (the notification channel registry, `oauth/oauth.pending-codes.ts`); + a second provider instance would hold a second, empty registry. +- No self-registration by side-effect import: the built-in notification + channels are built from injected transports in `NotificationsService`'s + constructor. +- A guard with a constructor dependency has to be a registered provider + wherever it is used, which is why the three auth guards stay dependency-free. -`permissions/trip-access.guard.ts` resolves `:tripId` once per request, answers 404 -"Trip not found" for anything the user cannot reach (never 403 — that would confirm the -id exists), and hands the row to the handler through `@Trip()`. `@RequirePermission` -carries the same action string the domain services pass to `checkPermission`. +## Tests -Two limits are worth knowing before rolling it onto another controller, because both -were found the expensive way: +Every module ships two kinds of tests. -- **A guard runs before the body pipe.** Any route whose DTO validation is expected to - answer 400 *ahead of* the trip 404 cannot use it. `places` is exactly that case — its - e2e suite documents the pipe-first ordering as a deliberate parity shift — so its - create/update/import routes keep their in-handler `requireTrip`, and that is not an - oversight. Its five body-free routes (list, get, unrate, image, delete) do carry - the guard; the split runs along "does this handler take a DTO-typed body", not - along the domain. -- **A guard also runs before interceptors.** On a multipart upload route, a 404 sent - while the client is still streaming destroys the socket, so the caller sees - ECONNRESET instead of the 404. The three upload routes (files, collab note files, - places) keep their own check for that reason; their controllers apply the guard per - handler instead of on the class. +1. **Unit**: `tests/unit/nest/.controller.test.ts` and + `.service.test.ts`. Build the class directly with its collaborators + (no `overrideProvider`); assert status codes, the exact `{ error }` bodies and + how inputs are forwarded. Parity assertions live here; there is no separate + parity directory. See `tests/unit/nest/weather.controller.test.ts`. +2. **e2e**: `tests/e2e/.e2e.test.ts` boots the module against a temp + SQLite through `tests/e2e/harness.ts` (`createTempDb`, `seedUser`, + `sessionCookie`) and runs the real `JwtAuthGuard`: 401 without a cookie, 200 + with a signed session. Mock external I/O only. See + `tests/e2e/weather.e2e.test.ts`. -`permissions/trip-owner.guard.ts` is the stricter sibling: `@RequireTripOwner(message)` -demands that the caller *owns* the trip, not merely that they may edit it. Handing a -trip over and creating or deleting guests are the two things a collaborator must never -do however generous the trip's permission settings are. +Seed and read rows through the ORM factories in `tests/helpers/factories/`; +raw `.prepare()` fixtures are legacy that `lint:test-sql` only lets shrink. -It deliberately does **not** inject `PermissionsService`. `checkPermission` returns true -for every admin, and the trip actions are admin-lowerable, so routing ownership through -it would quietly hand any admin the ability to transfer other people's trips. The check -is the literal one the routes did by hand: `trip.user_id === user.id`. The message -travels in the metadata because the two call sites answer with different strings and -both are asserted. +`tests/` is outside the build's `include`, so a constructor that grows a +parameter does not break a hand-wired `new` in a test at build time; +`npm run typecheck:tests` is the gate that catches it. -The services keep `verifyTripAccess`/`canEdit` regardless: most of their callers are -`*.mcp.ts` tools, which never pass through an HTTP guard. +**Coverage is a ratchet per domain.** `server/vitest.config.ts` holds one +threshold entry per `src/nest//` (floor 80 %, most pinned higher), and +`tests/unit/coverage-thresholds.test.ts` fails on a folder without one. After +a run that raised coverage, regenerate the block with +`node scripts/coverage-thresholds.mjs` (it reads the JSON coverage summary, +not the per-directory text report); never lower an entry. Only `src/nest/**` +is measured, so moving code into this tree starts measuring it: run +`npm run test:coverage` before pushing such a move. -## Coverage is gated per domain, as a ratchet +## Definition of done (per module) -`server/vitest.config.ts` carries one threshold entry per `src/nest//`, set at -that domain's measured coverage minus one point. The single repo-wide 80% it replaced -let a well-covered domain subsidise a thin one — `booking-import` sits at 50% and -`integrations` at 20%, and the aggregate still cleared the bar, so either could have -lost another ten points with the build staying green. - -Regenerate the block with `node scripts/coverage-thresholds.mjs` after a run that -RAISED coverage. Raise an entry when you improve a domain; never lower one to make a -build pass. The script reads `coverage/coverage-summary.json` rather than the text -reporter on purpose: the text reporter prints one row per DIRECTORY, so a domain with -subdirectories reads several points higher there than it actually is. - -## How to write the tests - -Every module ships two kinds of tests. The coverage gate (`vitest.config.ts`) -requires ≥80% over `src/nest/**` — note that it is an **average across the whole -tree**, not a per-file floor, so a large untested module can hide behind the -well-covered small ones. - -1. **Service / controller unit spec** — `tests/unit/nest/.controller.test.ts`. - Instantiate the controller with a mocked service; assert status codes, the exact - `{ error }` bodies, and that inputs are forwarded correctly (defaults, coercion). - See `weather.controller.test.ts`. - - Parity against the behaviour being replaced belongs here too — assert the exact - status codes and bodies the old path produced. There is no separate - `tests/parity/` directory; it was removed along with the routing toggle. - -2. **e2e** — `tests/e2e/.e2e.test.ts`. Boot the Nest module against a temp - in-memory SQLite db via the shared harness (`tests/e2e/harness.ts`: - `createTempDb`/`seedUser`/`sessionCookie`), exercising the **real** `JwtAuthGuard` - end-to-end (401 without cookie, 200 with a signed session). Mock external I/O - (HTTP/etc.). See `weather.e2e.test.ts`. - -## Definition of Done (per module) - -Contract in `@trek/shared` → service ported 1:1 → controller with identical routes → -validation/error parity → unit + e2e tests, with the old behaviour asserted in the -unit test → module registered in `app.module.ts` → **then** decommission the legacy -service (separate step) → frontend points at the typed contract (Frontend Track). - -## Migrating a legacy `src/services/*` service into its Nest module (recipe) - -**Historical record.** `src/services/` finished emptying in 2026-08 (see above); -the numbered steps below describe how each fold was actually done at the time, -including its `DatabaseService` construction — a deliberate changelog, not a -recipe to follow for new code. `DatabaseService` itself is gone (ORM migration -Plan 4): a service now takes the repositories it needs plus `UnitOfWork` for -transactions, per the root and server `CLAUDE.md`'s DB sections. - -Pilot: **tags** (`services/tagService.ts` → `nest/tags/tags.service.ts` + -`nest/tags/tags.bridge.ts`); categories followed the same shape (and piloted the -first `@Resource` in `categories.mcp.ts`); todo followed too (and piloted the -first `@ResourceTemplate` plus the `when` addon gate in `todo.mcp.ts`, and the -first in-container consumer wiring: `TripsService` injects `TodoService` via -`exports: [TodoService]` instead of using the bridge); packing followed (the -largest port so far: a 17-tool + 2-resource `packing.mcp.ts` with inline admin -gates, and the first `PluginHostDepsFactory` swap done as part of a service -migration — no bridge entry for the plugin host); day-notes followed (the first -migration needing **no bridge at all**: after its three tools + resource moved -to `day-notes.mcp.ts` and the plugin host injection, nothing outside the -container consumed it); trip-invite followed (the smallest port: no MCP -surface, no plugin-host import and no bridge — the SQL folded straight into -`trip-invite.service.ts`); assignments followed (a 7-tool `assignments.mcp.ts`, -the plugin-host swap, and a bridge kept only for the two legacy registrars — -places and reservations — that borrow its existence checks; the batch loaders -stay in `services/queryHelpers.ts`, shared with the unmigrated place -service); share followed (never imported by the plugin host, and its three MCP -tools stay in the legacy trips registrar — their `trips:share` scope gate has -no declarative `access: { group, mode }` equivalent — so the port is the SQL -fold plus a 3-export `share.bridge.ts` for `mcp/tools/trips.ts`); settings -followed (no MCP surface, no bridge, and the first migration that converted an -in-container plain-function consumer into a provider instead of bridging it: -`llm-parse/llm-config.resolver.ts` became the injectable `LlmConfigResolver`, -injected by `LlmParseService` and `PluginHostDepsFactory`); files followed (no -MCP surface and no addon gate; the load-time constants + pure helpers consumed -by three controllers' module-scope multer configs moved to -`files.constants.ts`, and `files.bridge.ts` survives with a single export — -the request-time `getAllowedExtensions` app_settings read those configs need -outside DI — while every function consumer, including the plugin host and -`TripsService`, injects `FilesService`); collab followed (a 12-tool + -3-resource `collab.mcp.ts` that piloted **composite `when` gates** — the collab -addon AND the per-sub-feature `getCollabFeatures()` flags (notes/polls/chat) -that the legacy registrar and resources checked at registration time — plus -the plugin-host swap and a 3-export `collab.bridge.ts` for the two remaining -legacy consumers, `tripService`'s trip summary and `mcp/tools/trips.ts`); -vacay followed (the largest MCP port yet — a 26-tool + 3-resource -`vacay.mcp.ts`, including the first fixed-URI `@Resource` behind a `when` -addon gate — plus the plugin-host swap and a 1-export `vacay.bridge.ts` for -`tripService`'s trip-window entry shift; the DTO ratchet for its 13 -grandfathered body contracts landed as a sibling commit); reservations -followed (the residue fold: the wrapper `ReservationsService` was already -DI-native at the edge, so the 626-line legacy module folded into it — a 5-tool -+ 1-resource `reservations.mcp.ts`, the plugin host's last plain-function -reservation import (`notifyBookingChange`) swapped for the injected service, -`TripsService` and `BookingImportService` converted from function imports to -injection, and a 9-export + 3-type `reservations.bridge.ts` for the legacy -`tripService`, the airtrail import/sync pair and the still-legacy transports -registrar (the transit registrar has since migrated to `transit.mcp.ts`); the -DTO ratchet for its 4 grandfathered body contracts -landed as a sibling commit, which also loosened the shared positions schema to -the real wire contract — `day_plan_position` optional, pinned by RESV-006); -day followed (the 592-line legacy `dayService` folded into the existing -wrapper `DaysService` — including the accommodation SQL that -`nest/reservations/accommodations.service.ts` now reaches via an injected -`DaysService` — with the hand-rolled `BEGIN`/`COMMIT` blocks in -reorder/insert converted to `db.transaction()`; a 7-tool + 2-resource -`days.mcp.ts`, the plugin host's 11-symbol import swapped for the injected -service, `TripsService` + the assignments/reservations MCP controllers -converted to injection, and a 6-export `days.bridge.ts` for the legacy -`tripService` and the still-legacy transports registrar (transit has since -migrated); the DTO -ratchet for its 4 day + 2 accommodation grandfathered body contracts landed -as a sibling commit); -permissions followed (the first Wave-2 **cross-cutting** migration and the -first greenfield module in the series — no prior wrapper, controller, MCP -surface or DTO of its own: a new `nest/permissions/` whose -`PermissionsService` is injected by 16 domain services, the airtrail-import -controller and `PluginHostDepsFactory` (its 16th constructor dep) in one move, -plus a 5-function `permissions.bridge.ts` for `mcp/tools/_shared.ts` — one -repoint covering every `hasTripPermission` call site — and the four legacy -consumers adminService/authService/backupService/collectionsService; the -permissions **cache stays module-scoped** in `permissions.service.ts` on -purpose, so the bridge instance and the container singleton share one cache -and backup-restore's bridge-side `invalidatePermissionsCache()` flushes what -request handlers serve; the domain e2e suites swapped their path mocks for a -`vi.spyOn(app.get(PermissionsService), 'checkPermission')` instance spy); -auditLog followed (the other Wave-2 half, split into five files: the -injectable `AuditService` (`writeAudit` over `DatabaseService`), the pure -`client-ip.ts` (files.constants precedent — the four getClientIp-only -controllers stay plain imports), the deliberately side-effectful plain -`audit-log.logger.ts` — frozen-at-import `LOG_LEVEL` and the import-time -`data/logs` mkdir are a documented parity exception to the no-side-effects -rule because `index.ts` lazy-requires it pre-container and tests/setup.ts -sets the level pre-import — plus `audit.module.ts` and a full-surface -`audit.bridge.ts` for `mcp/index.ts`, `mcp/oauthProvider.ts` and the legacy -airtrail/immich/oauth services, while log*-only consumers (index.ts's lazy -require strings, scheduler, globalMiddleware, notifications) point at the -logger directly; 8 controllers + `PluginRuntimeService` inject `AuditService`, -and the domain e2e suites dropped the audit mock entirely — writeAudit runs -for real against an `audit_log` table in their temp DBs). -The exchangeRateService fold followed (the pure-infra FX helper — Frankfurter -fetch + module-scoped 6h cache, no SQL, no controller, no MCP registrar of its -own — folded into `nest/budget/` as a dep-free `ExchangeRatesService`, injected -by `BudgetService` and `PluginHostDepsFactory` (its 17th constructor dep); the -rate **cache stays module-scoped** like the permissions cache, so any -out-of-container instance and the container singleton share one cached -upstream feed). -budgetService followed (the 755-line Wave-4 money core folded into the wrapper -`BudgetService`: items/members/payers CRUD, the FX freeze + rebase paths and -the settlement maths with `splitEqualShares` gone private; the freeze-then-write -composites kept their wrapper names while the raw settlement writes became -`insertSettlement`/`applySettlementUpdate` so the MCP paths keep skipping the -freeze; the 11-tool `mcp/tools/budget.ts` registrar + 3 `resources.ts` budget -resources moved to `budget.mcp.ts`; TripsService, ReservationsService (+ its -MCP class) and BookingImportService inject `BudgetService`; a 4-export -`budget.bridge.ts` served the legacy tripService/userCleanupService and the -trips/transports registrars (deleted 2026-08-11: BudgetMcp's demo guard moved -off AuthService onto `isDemoUserId`, BudgetModule dropped AuthModule, and -`UserCleanupService` injects `BudgetService` now); -`exchange-rates.bridge.ts` was deleted with its -last consumers, and the controller adopted `budget.dto.ts` — all nine -allow-list entries removed). -tripService followed (the 1121-line Wave-4 hub — the biggest fold — moved into -the wrapper `TripsService`: TRIP_SELECT + list/create/get, the -`generateDays` two-phase renumber engine, the updateTrip date-shift -transaction, the member/guest lifecycle (#973/#1362), the ICS export with its -module-scoped tz-validity cache, `copyTripById` and `getTripSummary`; its six -bridge imports became injected services (CollabService + VacayService joined -the constructor); the 10-tool trips registrar + 3 `resources.ts` trip -resources + the trip-summary prompt moved to `trips.mcp.ts` — the first -`@Prompt` use, with the fire-once static-token deprecation notice now riding -the `registry.attach` ctx — and the 3 share-link tools it carried moved to -`share.mcp.ts` on the `canShareTrips` predicate (delete_trip and the -canReadTrips reads are predicates too — the broadened legacy gates have no -declarative equivalent); FeedsService and the plugin host inject -`TripsService` (its 20th constructor dep); a 4-export `trips.bridge.ts` -serves budget.mcp.ts / packing.mcp.ts / costs.rpc.ts (verified-permanent -2026-08-11: the trip read-model/members modules import the budget and packing -domains, so injecting any backing service there closes a real cycle; the -prompts registrar it also served is long migrated); -`todo.bridge.ts`, `share.bridge.ts`, `collab.bridge.ts` and `vacay.bridge.ts` -were deleted with their last consumers and the unused days/budget bridge -exports pruned; the controller adopted `trips.dto.ts` — all seven allow-list -entries removed). -mapsService followed (the 1429-line geo core — Google Places, Nominatim, -Overpass mirror racing, Wikimedia photos, Maps-URL resolution — folded into the -wrapper `MapsService`; the pure parser/UA/POI-category helpers moved to -`maps.helpers.ts` as plain exports (files.constants/client-ip precedent — -the DI-native TransitService's User-Agent imports from there, not a bridge), the module-scoped -POI cache / photo-fetch semaphore / frozen Overpass mirrors stayed module-scoped -on purpose (permissions-cache precedent: any out-of-container instance and the -DI singleton share them); the 3 geo tools left the mixed `mcp/tools/mapsWeather.ts` -registrar for the decorator-driven `maps.mcp.ts` (the registrar file survives — -its weather + airport tools await their own migrations); BookingImportService -injects `MapsService` for its Nominatim geocoding, and a 3-export -`maps.bridge.ts` served the legacy placeEnrichment helper and the places -registrar — both absorbed by the 2026-07 place fold, which deleted the bridge). -transitService followed (the first fully SQL-free domain fold — the 333-line -Transitous/MOTIS proxy became a dep-free `TransitService` (no -`DatabaseService`; the ExchangeRatesService precedent), its response cache, -frozen-at-import `TRANSIT_API_BASE` and lazy User-Agent memo staying -module-scoped on purpose; the pure `deriveTransitStats` + mode whitelist + -itinerary types moved to `transit.helpers.ts` (maps.helpers precedent) so the -downstream legacy `transitItineraryService` needed no bridge (since relocated -into the domain as `transit-itinerary.helpers.ts`); the whole 3-tool -`mcp/tools/transit.ts` registrar moved to `transit.mcp.ts` — the two geo -search tools on `access: { group: 'geo', mode: 'read' }` and -`create_transit_journey` on `reservations:write`, with its days/reservations -bridge imports becoming injected `DaysService`/`ReservationsService` (+ -`DatabaseService` for `canAccessTrip`) — leaving **zero bridge files** and the -transports registrar as `days.bridge.ts`'s last consumer). -placeService followed (the step-4 tail: the 1029-line place core — the -CRUD + ratings SQL, the GPX/KML/KMZ importers and the Google/Naver list -importers — folded into the wrapper `PlacesService`, with the pure pieces -(frozen XML parsers, the KMZ unpacker, the dedup predicates, the Google -hex-id parsers, `reclaimPhotoCache`) moving to `places.helpers.ts` on the -maps.helpers precedent; the 10-tool `mcp/tools/places.ts` registrar + the -`trek://trips/{tripId}/places` resource moved to `places.mcp.ts` — -`search_place` came along because its gate is `places:read`, not the geo -group, and now injects `MapsService`; TripsService, DaysMcp, -BookingImportService and the plugin host (its 21st constructor dep) inject -`PlacesService`, leaving **zero bridge files** for the domain; the two -`assignments.bridge` imports stay in `places.mcp.ts` on purpose — -AssignmentsModule imports DaysModule and DaysModule now imports -PlacesModule, so injecting would close a -DaysModule → PlacesModule → AssignmentsModule → DaysModule cycle -(reservations.mcp.ts uses the same seam for the same reason). The sibling -`placeEnrichment` fold went further than the recipe's minimum: the 168-line -helper's DB/websocket/Maps half became `PlacesService` methods over the -injected `DatabaseService`/`RealtimeService`/`MapsService` and its pure -match selector joined `places.helpers.ts`, which retired **`maps.bridge.ts`** -with its last consumer. The DTO ratchet for its 7 grandfathered body -contracts landed as a third commit, which also loosened -`placeBulkUpdateRequestSchema.ids` (`.min(1)` dropped) so the endpoint's -empty-list short-circuit stays reachable, and retired three bespoke 400 -strings — 'Place name is required', 'ids must be an array of numbers' and -'URL is required' — in favour of the pipe envelope, accepting that a -malformed body now 400s ahead of the trip-access 404 (the todo/trips trade). -transitItineraryService followed (the first pure-helpers relocation with no -service fold at all — the 287-line module is 100% pure, so the recipe's SQL / -bridge / DTO / plugin-host steps were all no-ops: the Zod itinerary schemas + -endpoint/metadata builders moved byte-identical to -`transit-itinerary.helpers.ts`, next to `transit.helpers.ts`; the schemas -**must** stay module-level plain exports because `transit.mcp.ts` consumes -them inside `@Tool({ inputSchema })` decorators, which evaluate at module load -before any container exists; the sole consumer — the in-container -`transit.mcp.ts` — was a one-import repoint, closing step 4 of the -dependency-honest order; the legacy module had no direct suite, so a new -21-case `TRANSIT-ITIN-*` characterization suite now pins all 12 superRefine -error strings, the `??` time fallbacks, the coordinate tolerances and the -reservation endpoint/metadata builder). -collectionsService followed (the biggest single fold yet — 1024 lines / 28 -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` — 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 -didn't relocate verbatim). -atlasService followed (1612 lines split two ways, places precedent: the DB -half — stats aggregation, visited countries/regions with the #1490 -tombstone/cascade logic, bucket-list CRUD — folded into `AtlasService` over -DatabaseService alone, while the ~750-line pure-geo half — the bundled -admin0/admin1 stores and their #1576 OOM-shaped streaming builders, the -point-in-polygon indexes, Nominatim geocoding with its shared ≥1.1s throttle, -the 50k geocode cache with its import-time unref'd cleanup interval, and the -`geocodingInFlight` dedup set — moved to the plain module `atlas-geo.ts` so -those caches stay process-global across the container instance, the bridge -instance and test helpers; `assetPath` re-anchored one directory deeper, the -only non-verbatim line. The 10-tool legacy registrar `mcp/tools/atlas.ts` -plus all four atlas resources in `mcp/resources.ts` (`trek://bucket-list`, -`trek://visited-countries`, `trek://atlas/stats`, `trek://atlas/regions`) -moved to `atlas.mcp.ts` — here the `when:` atlas-addon gate IS parity, since -the legacy registrar and resources both gated on the addon while the REST -controller deliberately does not; the mark_region_visited / -get_country_atlas_places no-uppercase divergence from REST relocated -untouched. The plugin host swapped its 9 atlas imports for the injected -service — its 23rd constructor dep — and a minimal 2-export `atlas.bridge.ts` -existed solely for the legacy `authService.getTravelStats` edge -(`getCountryFromCoords` re-exported straight from atlas-geo, -`getHiddenCountries` over the bridge instance); it died with the auth fold, as -predicted. resources.test.ts retired with its last two cases — every resource -it covered now lives in the domain suites. The trailing `fix(server)` quirk -commit then wrapped the four multi-statement mark/unmark writes in -`db.transaction()` (the region cascade nests as a savepoint), made the -trip-less `countryPlaces` early return honour `manually_marked`, fixed the -`|| null` bindings that dropped `lat: 0`/`lng: 0`/`notes: ''` on bucket -update, user-scoped the mutating bucket SQL, and uppercased the MCP -region/country-places codes to match REST — each pinned by -`ATLAS-SVC-031…036` plus two MCP casing cases; see migration-graph.md's -"Quirks fixed after the atlas fold".) -authService followed (1497 lines, the biggest fold and the head of the -auth → admin → oauth chain: the pure crypto half — backup-code -hash/match/generate, `stripUserForClient`, key masking, the import-time -`DUMMY_PASSWORD_HASH` timing equaliser and the `avatarDir` mkdir (both -documented parity exceptions) — moved to the plain module `auth.helpers.ts`, -while the DB half — toggles/app-config, register/login with the CWE-208 -dummy-hash path, MFA setup/enable/disable/verify, the password-reset -throttle+token flows with their two `db.transaction()` revocation blocks, -API keys/settings, travel stats, MCP/ws/resource tokens, `isDemoUser` and -the two token verifiers — folded into `AuthService` over DatabaseService + -injected PermissionsService (ex `permissions.bridge`) and AtlasService (ex -`atlas.bridge`, which died on schedule). The `mfaSetupPending` and -per-email reset-throttle maps stay module-scoped (with the unref'd cleanup -interval) so the bridge instance and the container singleton share them; the -otplib `window: 1` mutation runs at module top; `require('../../../package.json')` -and `avatarDir` re-anchored one directory deeper — the only non-verbatim -lines besides the two injection swaps. No MCP registrar existed and the -plugin host never imported authService, so neither surface changed; the 15 -in-container `*.mcp.ts` demo guards now inject AuthService (their modules -import AuthModule) — except `atlas.mcp.ts`, which keeps a bridge import -because AuthService injects AtlasService and the reverse module edge would -close a cycle (places.mcp precedent). The 8-export `auth.bridge.ts` serves -`mcp/index.ts` token verification, the three legacy tool registrars' -`isDemoUser`, and the un-migrated adminService. -Tests moved with IDs preserved: authService.test.ts → auth.helpers.test.ts, -authServiceDb.test.ts → auth.service.test.ts (+ AUTH-BR-001…007 bridge -delegation); auth.e2e converted DI-native — the 30-method whole-module mock -died, login now runs real bcrypt and real audit rows; oidc.e2e switched its -dead path-mock for a `vi.spyOn(app.get(AuthService), …)` instance stub.) -oidcService followed (the frontier cash-in the auth fold predicted: a pure -fold of the 508-line module into the existing wrapper `OidcService` over -DatabaseService + injected AuthService — the `resolveAuthToggles` -`auth.bridge` import became the injection, the first repoint-consumer of that -bridge to migrate. No MCP registrar, no plugin-host import, and **no bridge -at all**: nothing outside the container consumes the domain (day-notes/ -trip-invite class), so the `pendingStates`/`authCodes` maps, their two sweep -`setInterval`s, the single-slot discovery cache and the JWKS cache became -**instance state** — the module-scope precedent (permissions/atlas-geo/auth -maps) applies only where a bridge instance must share state, and none exists -here; the sweepers start in the constructor and are cleared in -`onModuleDestroy`, the one wire-invisible deviation. The `uuid`/`bcryptjs` -lazy requires, the `invite_exhausted` reference-sentinel transaction (reshaped -one line for `DatabaseService.transaction`) and every SQL string relocated -verbatim; controller and its unit suite needed zero edits. Tests moved with -IDs preserved: oidcService.test.ts → oidc.service.test.ts (OIDC-SVC-001…045, -superseding the 18-case delegation-shim suite; 046–048 carry its wrapper -cases); oidc.e2e swapped its whole-module path mock for -`vi.spyOn(app.get(OidcService), …)` instance spies; the integration suite -spies the four HTTP methods on the container instance while driving the real -state maps on that same instance.) -passkeyService followed (the last frontier member of the auth stack: the -364-line WebAuthn module folded into a new `nest/auth/passkey.service.ts` -`PasskeyService` over DatabaseService + injected AuthService — its three -`auth.bridge` imports resolved to `this.auth.generateToken` plus plain -`stripUserForClient`/`avatarUrl` helper imports, and `resolveWebauthnConfig` -stayed a plain `services/webauthnConfig` import at the time (it has since -become the injected `WebauthnConfigService`, which also feeds auth.service's -`passkey_configured`). No MCP registrar, no plugin-host -import, and **no bridge at all** — both consumers were already in-container: -`PasskeyController` swapped its `import *` shim for the injection, and -`AdminService` swapped its `adminResetPasskeys` function import for the -injected service (`AuthModule` now exports PasskeyService and `AdminModule` -imports AuthModule — the todo→TripsService in-container precedent). No -module-level mutable state existed to preserve: the challenge store is -DB-backed (`webauthn_challenges`, single-use `DELETE … RETURNING` claim, -5-min TTL), so the fold is a plain stateless injectable — every SQL string, -error string and the counter/login-bookkeeping transaction relocated -verbatim. Tests: the legacy module had **no service-level suite**, so -PASSKEY-SVC-001…030 were written fresh with the fold (characterization over a -real `:memory:` DB, `@simplewebauthn/server` mocked at the ceremony-verdict -boundary — the repo's first such mock); auth-guard.test.ts's PasskeyController -block swapped its path mock for a constructor stub.) -notificationService followed (the notifications fan-in, step 3 of the -dependency-honest order: the `send()` dispatcher **and** the -`inAppNotifications.ts` store folded together into the existing -`NotificationsService` over DatabaseService + RealtimeService — the wrapper's -in-app delegation became the real SQL, while the prefs matrix, the smtp/ -webhook/ntfy transports, the channel registry and `inAppNotificationActions` -stay plain-module imports (graph-classified infra helpers, complete with -their registry⇄prefs cycle). A one-export `notifications.bridge.ts` (`send`) -covered the outside-container consumers — scheduler's two cron `require`s, -legacy adminService and memories/{unified,synology} (that bridge died -2026-08-10 with the scheduler migration; `notifications.instance.ts` carries -the surviving cycle-dodge bridges) — **and** the six -deliberately-lazy fire-and-forget `import().then(({ send }) => …)` sends in -migrated Nest services (collab/collections/packing/reservations/trips/vacay, -path-only repoints; kept lazy so a send can never block or cycle a domain -module). In-container static consumers inject instead: `AdminController`'s -dev test send (AdminModule → NotificationsModule) and the plugin host -(`NotificationsService` = `PluginHostDepsFactory`'s 24th dep). The 5-tool -legacy registrar + the `notifications-in-app` resource moved 1:1 to -`notifications.mcp.ts` (no `when:` — the domain is core, not addon-gated). -Tests moved with IDs preserved: notificationService.test.ts → -notifications.service.test.ts (NSVC-001…019/NTFY-SVCB-*/NSVC-PLUG-001…007, -plus the NSVC-020 bridge pin), inAppNotificationPrefs.test.ts → -notifications.inapp-prefs.test.ts (INOTIF-*); ~18 suites repointed their -path mocks/warm-ups to the bridge; admin.controller.test and the plugin-host -suite converted theirs to constructor stubs; the module e2e went DI-native -(real notifications DDL, prefs/transports still path-mocked).) -adminService followed (the last Wave-5 god file and the first fold where recipe -steps 2-4 were all no-ops or near-no-ops: **no `src/mcp/tools/admin.ts` has ever -existed** — the "11 MCP consumers" figure carried in `migrate.md` / -`migration-graph.md` predated the Phase-0 addons extraction, so no registrar -moved, `mcp/tools.ts` / `mcp/resources.ts` / `mcp-test-controllers.ts` were -untouched — and `plugin-host-deps.factory.ts` never imported the domain either -(its addon reads already went through the DI-native `AddonsService`). The -851-line module folded into the wrapper `AdminService` over `DatabaseService` -plus injected Settings/Addons/Passkey/Packing/Auth/Permissions/Notifications -services: the `auth.bridge` (`resolveAuthToggles`), `notifications.bridge` -(`send`) and `permissions.bridge` (`getAllPermissions`/`savePermissions`) -imports all became injections, while `PERMISSION_ACTIONS` stayed a plain const -import and the `mcp/sessionManager` deep import kept its anti-cycle comment. -The pure + module-scoped half moved to `admin.helpers.ts` — `compareVersions`, -`utcSuffix`, `BCRYPT_COST`, the import-time `isDocker` probe (a documented -parity exception, auth.helpers precedent) and the 5-minute version cache, which -stays module-scoped so the bridge instance and the container singleton share it. -Ahead of the fold the 11 packing-template functions relocated to -`PackingService`, which already owned all three template tables -(`saveAsTemplate` writes every one of them) — that also resolved the `admin-2` -residual without a bridge, since `packing.mcp.ts` already injects the service; -`AdminModule` gained PackingModule and PermissionsModule, both cycle-free. -A 1-export `admin.bridge.ts` (`checkAndNotifyVersion`) served the only -out-of-container consumer, `scheduler.ts`'s daily cron (both died 2026-08-10 -with the scheduler migration — `VersionCheckJob` injects `AdminService`). -Four lines are -non-verbatim, all path re-anchoring one directory deeper for `nest/admin/` -(`rotateJwtSecret`'s data dir, the `package.json` version require, the -websocket/demo-reset lazy requires) — both resolved paths verified against the -emitted `dist/` layout. Tests moved with IDs preserved (adminService.test.ts → -`nest/admin.service.test.ts`, ADMIN-SVC-001…069 incl. the pre-existing 029/030 -gap and duplicated 069, + ADMIN-BR-001 pinning the bridge and the shared version -cache; versionNotification.test.ts → `nest/admin.version-notification.test.ts`, -VNOTIF-001…007; the template cases rode along to `packing.service.test.ts`), and -the module e2e went DI-native — its 3-method whole-module mock died, 6 cases -became 15 over real SQL. A sibling DTO ratchet cleared all twenty -`AdminController` allow-list entries — the largest single block — trading the -three `'enabled must be a boolean'` checks plus `'permissions object required'` -and `'Object body required'` for the pipe envelope; the schemas are -deliberately permissive wherever the service owns a bespoke 400 of its own.) -Repeat these steps per -service (next up: **backupService** / **airtrail** — -per the dependency-honest order in -`migration-graph.md`). This is a -**pure relocation** — byte-identical -SQL, statuses, bodies, and error strings. The plugin RPC host is **no longer a -bridge consumer**: since Option A of `src/nest/plugins/DI-MIGRATION.md` it -injects domain services via `PluginHostDepsFactory`, so a migrated domain adds -`exports: [XService]` + a `PluginsModule` import instead of a bridge entry. -Only the pre-`app.init()` MCP/OAuth mount still needs bridges — the module- -cycle seams are folded, nothing on the websocket path imports one, and the -crons all inject now (`*.job.ts` providers on `scheduling/CronRegistrarService`). - -1. **Move the SQL** into `.service.ts` as methods over an injected - `DatabaseService` (`this.db.all/get/run/prepare`; multi-statement writes go - through `await this.uow.transactional(...)` instead — see the root `CLAUDE.md`; - strict constructor injection, no `@Optional()`). Preserve every quirk: falsy-coercion - defaults (`x || fallback`, never `??`), post-insert/post-update re-selects (no - RETURNING), `COALESCE` semantics. If a controller already wraps the legacy - functions, do not change the service's method surface. The module needs no - `imports: [DatabaseModule]` — it's `@Global`. -2. **Add `.bridge.ts`** next to the service **only if non-Nest consumers - exist** (legacy MCP tool registrars, websocket — the plugin RPC host and the - crons now inject instead, see above). It builds a module-level instance over - the shared connection Proxy — `new XService(new DatabaseService(db))`, - reinitialize-proof, same pattern as `nest/todo/todo.bridge.ts` — and exports - the legacy function names 1:1. - Container code injects the service; only outside-container code imports the - bridge. When porting an MCP registrar, note the `access: { group, mode }` - markers are typed against the scope-derived `ScopeGroup` union and - boot-validated by `trekMcpValidateAccess` (`src/mcp/nest-mcp-policy.ts`) — - an unknown group, or `mode: 'write'` on a read-only group (`geo`, - `weather`), fails app boot. **`mode` is typed the same way**: it is the mode - half of `Scope` (`read | write | delete | share`), fed into - `src/nest-mcp/types.ts` through `McpAccessModeRegistry` exactly like the - group registry. A scope that is neither read nor write — `journey:share` is the - one today — gets a real marker, *not* a `(ctx) => canX(ctx.scopes)` - predicate: predicates bypass the policy entirely and are invisible to the - boot gate, so a scope typo in one would ship as a silent full-access tool. - Reach for a predicate only when the gate genuinely is not a scope. *(Design decision, settled with the tags pilot: - MCP tools stay outside the container and use the bridge. The alternative — handing the Nest app to the - MCP layer via `app.get(XService)` — was rejected: it would thread the container - through `mcpHandler` + every tool registrar and force a Nest bootstrap into the - container-less `tests/helpers/mcp-harness.ts` used by ~25 MCP suites.)* -3. **Repoint non-Nest consumers** — import-path-only diffs from - `services/Service` to `nest//.bridge`; call sites unchanged. -4. **Delete the legacy service** once `grep -rn "services/Service" src tests` - only hits the tests you're rewriting. -5. **Tests:** - - Move `tests/unit/services/Service.test.ts` → - `tests/unit/nest/.service.test.ts`, preserving case IDs. Construct the - service directly — `new XService(new DatabaseService(testDb))` — no - TestingModule, no `overrideProvider` (repo convention). Add one delegation - case per bridge export: the bridge sits under `src/nest/**`, inside the ≥80% - coverage gate, and these cases pin it deterministically. - - Convert the module e2e to the DI-native pattern (exemplar - `tests/e2e/trips.e2e.test.ts`): temp-db DDL for the domain's tables, - `imports: [DatabaseModule, Module]`, real SQL assertions. Keep only - the `vi.mock('../../src/db/database', …)` — the auth guard still reads users - through the singleton, and `DatabaseModule`'s factory picks up the same - mocked db. Drop the legacy-service mock entirely. - - Suites that mocked the legacy module mock the bridge path instead — same - factory shape, path-only change. (For the plugin host suite, - `plugin-host-deps.factory.test.ts`, the domain becomes a constructor stub - instead of a path mock.) -6. **Verify** (from `server/`): `npm run typecheck`, `test:unit`, - `test:integration`, `test:e2e`, `lint:check`, `test:coverage`. +Contract in `@trek/shared` → service over injected repositories → controller +with the routes, statuses and bodies of the contract → the parallel MCP tool on +the same service and permission → unit and e2e tests → module registered in +`app.module.ts` → the client calls it through the typed contract. diff --git a/wiki/Addons-Overview.md b/wiki/Addons-Overview.md index f173e4953..0ed7bc6aa 100644 --- a/wiki/Addons-Overview.md +++ b/wiki/Addons-Overview.md @@ -10,7 +10,7 @@ Each addon extends TREK with functionality beyond the core trip-planning feature ## Addon list -The following addons are registered in the system (defined in `server/src/db/seeds.ts`; the TypeScript constant `ADDON_IDS` in `server/src/addons.ts` covers all addons except `naver_list_import`): +The following addons are registered in the system (defined in `server/src/db/seeders/AddonSeeder.ts`; the TypeScript constant `ADDON_IDS` in `server/src/addons.ts` covers all addons except `naver_list_import`): | Addon ID | Type | Description | |---|---|---| diff --git a/wiki/Development-environment.md b/wiki/Development-environment.md index 334ffd126..5a9788411 100644 --- a/wiki/Development-environment.md +++ b/wiki/Development-environment.md @@ -212,7 +212,7 @@ The `@trek/shared` package is the single source of truth for code shared between | `npm run theme:lint` | Flag styling that bypasses the appearance tokens (not run in CI) | | `npm run check:gl-split` | Fail when one built chunk carries both map engines (MapLibre and Mapbox); run after a build | | `npm run build:analyze` | Production build with the bundle analyzer | -| `npm run e2e` | Playwright end-to-end tests (local only, not in CI) | +| `npm run e2e` | Playwright end-to-end tests; CI runs the public and app projects in Chromium and WebKit, the screenshot and help-media projects are local only | | `npm run shots` | Capture the wiki screenshots with Playwright | | `npm run shots:promote` | Downscale the captured screenshots and move them into `wiki/assets/` | | `npm run help:media` | Record the help-center pictures against the real app on its own ports, so `npm run dev` can keep running |