From 8e4261ed81fab511b39d5a58c7aeecbebdae9548 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 23:40:53 +0200 Subject: [PATCH] Delete four bridges with no consumer, and gate what runs outside the container Both were left open by the audit and are closed here. days, packing, photos and reservations each built a service outside the container at module load for nobody: only their own delegation tests imported them, and three domain docblocks were still telling readers to go through them. photos.bridge's own comment pointed at src/services/, a directory deleted two PRs ago. The cases that exercised them kept their assertions and call the service the bridge delegated to, so nothing that was proved stopped being proved. Twelve bridges remain, each with a live out-of-container caller. Coverage measured src/** but gated only src/nest/**, so the boot path, the scheduler, the MCP handler and the migrations sat under no floor at all. That is the same 9,602 lines where this audit found its worst defect, and a swallowed EADDRINUSE is exactly the kind of thing an untested boot path hides. Seven globs now pin what is measured today. They are floors to raise, not targets that have been met: src/*.ts is at 39% statements and src/db at 38% branches, and writing those numbers down is the point. src/demo/** is deliberately left out. It measures 0%, and a floor of zero asserts nothing; it needs tests before it needs a threshold. --- server/src/nest/README.md | 13 ++-- server/src/nest/days/days.bridge.ts | 32 -------- server/src/nest/days/days.service.ts | 5 +- server/src/nest/packing/packing.bridge.ts | 27 ------- server/src/nest/packing/packing.service.ts | 4 +- server/src/nest/photos/photos.bridge.ts | 37 ---------- .../nest/reservations/reservations.bridge.ts | 74 ------------------- .../nest/reservations/reservations.service.ts | 8 +- .../integration/memories-synology.test.ts | 10 ++- server/tests/unit/nest/days.service.test.ts | 7 +- .../tests/unit/nest/packing.service.test.ts | 6 +- .../unit/nest/reservations.service.test.ts | 25 ++++++- .../unit/services/trekPhotoMedia.test.ts | 11 ++- server/vitest.config.ts | 20 +++++ 14 files changed, 86 insertions(+), 193 deletions(-) delete mode 100644 server/src/nest/days/days.bridge.ts delete mode 100644 server/src/nest/packing/packing.bridge.ts delete mode 100644 server/src/nest/photos/photos.bridge.ts delete mode 100644 server/src/nest/reservations/reservations.bridge.ts diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 84897ac3c..1c8b324f7 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -262,11 +262,14 @@ predicted: `MulterModule.registerAsync` over an `AllowedFileTypesService` leaf, and `files.controller.ts` / `journey.controller.ts` build their multer options from that. -**Four bridges have no production consumer at all** — `days`, `packing`, -`photos`, `reservations`. Only their own delegation tests import them, and each -still builds a service outside the container at module load for nobody. They are -deletable, along with the comments in `days.service.ts`, `packing.service.ts` and -`reservations.service.ts` that still send readers to them. +`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. + +Twelve bridges remain. 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 diff --git a/server/src/nest/days/days.bridge.ts b/server/src/nest/days/days.bridge.ts deleted file mode 100644 index afe9abae1..000000000 --- a/server/src/nest/days/days.bridge.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { db } from '../../db/database'; -import { DatabaseService } from '../database/database.service'; -import { RealtimeService } from '../realtime/realtime.service'; -import { DaysService } from './days.service'; -import { PermissionsService } from '../permissions/permissions.service'; -import { QueryHelpersService } from '../query-helpers/query-helpers.service'; - -/** - * Non-Nest entry point for the day domain — for code running OUTSIDE the - * Nest container (the legacy transports MCP registrar in src/mcp/tools/; - * the day MCP tools and resources moved to the DI-discovered days.mcp.ts, - * the transit tools to the DI-discovered transit.mcp.ts (which injects - * DaysService), the plugin RPC host injects DaysService via - * DaysRpc / AccommodationsRpc, and the trip domain folded into the DI-native - * TripsService, which injects DaysService — its bridge exports were pruned - * with it). Exports only the legacy services/dayService names still consumed - * outside the container, 1:1, so repointing a consumer is an import-path-only - * diff. Inside the container, inject DaysService instead. Delete this file - * when the transports registrar migrates. - * - * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton. - */ -const days = new DaysService(new DatabaseService(db), new PermissionsService(new DatabaseService(db)), new RealtimeService(), new QueryHelpersService(new DatabaseService(db))); - -export function getDay(id: string | number, tripId: string | number) { - return days.getDay(id, tripId); -} - -export function listDays(tripId: string | number) { - return days.list(tripId); -} diff --git a/server/src/nest/days/days.service.ts b/server/src/nest/days/days.service.ts index daf2e54df..0b0942136 100644 --- a/server/src/nest/days/days.service.ts +++ b/server/src/nest/days/days.service.ts @@ -56,9 +56,8 @@ export class DayReorderError extends Error {} * query per assignment. Trip access * rides DatabaseService.canAccessTrip; mutations use the * 'day_edit' permission; the WebSocket broadcast keeps its legacy call path. - * Non-Nest consumers (legacy tripService and the transit/transports MCP - * registrars) go through days.bridge.ts instead of importing this class - * directly. + * There are no non-Nest consumers left. days.bridge.ts served them and was + * deleted once the last one folded into the container. */ @Injectable() export class DaysService { diff --git a/server/src/nest/packing/packing.bridge.ts b/server/src/nest/packing/packing.bridge.ts deleted file mode 100644 index d67c1613e..000000000 --- a/server/src/nest/packing/packing.bridge.ts +++ /dev/null @@ -1,27 +0,0 @@ -import { db } from '../../db/database'; -import { DatabaseService } from '../database/database.service'; -import { RealtimeService } from '../realtime/realtime.service'; -import { PackingService } from './packing.service'; -import { PermissionsService } from '../permissions/permissions.service'; -import { notificationsInstance } from '../notifications/notifications.instance'; - -/** - * Non-Nest entry point for the packing domain — for code running OUTSIDE the - * Nest container (the legacy packing-list prompt registrar in - * src/mcp/tools/prompts.ts; the packing MCP tools and resources moved to the - * DI-discovered packing.mcp.ts, the plugin RPC host injects PackingService - * via PackingRpc, and the trip domain folded into the DI-native - * TripsService, which injects PackingService). Exports only the legacy - * services/packingService names still consumed outside the container, 1:1, so - * repointing a consumer is an import-path-only diff. Inside the container, - * inject PackingService instead. Delete this file when the prompts registrar - * migrates. - * - * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton. - */ -const packing = new PackingService(new DatabaseService(db), new PermissionsService(new DatabaseService(db)), new RealtimeService(), notificationsInstance()); - -export function listItems(tripId: string | number, userId?: number) { - return packing.listItems(tripId, userId); -} diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 16665994b..c55c04549 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -35,8 +35,8 @@ const BAG_COLORS = ['#6366f1', '#ec4899', '#f97316', '#10b981', '#06b6d4', '#8b5 * legacy call paths. Post-migration fixes over the legacy code: a single * 'Other' category default, bodyKeys-gated weight_limit_grams (explicit null * clears it), and transactions around every multi-statement write. The - * remaining non-Nest consumer (the legacy MCP prompts registrar) goes through - * packing.bridge.ts instead of importing this class directly. + * remaining non-Nest consumer went with the legacy MCP prompts registrar, and + * packing.bridge.ts was deleted with it. */ @Injectable() export class PackingService { diff --git a/server/src/nest/photos/photos.bridge.ts b/server/src/nest/photos/photos.bridge.ts deleted file mode 100644 index 50775dd06..000000000 --- a/server/src/nest/photos/photos.bridge.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { db } from '../../db/database'; -import { DatabaseService } from '../database/database.service'; -import { TrekPhotosRepository } from './trek-photos.repository'; - -/** - * Non-Nest entry point for the `trek_photos` store — for the modules still - * living in `src/services/` that only need to register or drop a row: - * journeyService (photo upload, Immich sync, entry cleanup) and the memories - * unifiedService. Exports the legacy photoResolverService names 1:1, so - * repointing a consumer is an import-path-only diff. Inside the container, - * inject TrekPhotosRepository instead. Delete this file when journeyService and - * the memories cluster fold. - * - * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton, and the repository holds no state. - */ -const photos = new TrekPhotosRepository(new DatabaseService(db)); - -export function getOrCreateTrekPhoto(...args: Parameters) { - return photos.getOrCreate(...args); -} - -export function getOrCreateLocalTrekPhoto(...args: Parameters) { - return photos.getOrCreateLocal(...args); -} - -export function resolveTrekPhoto(photoId: number) { - return photos.resolve(photoId); -} - -export function setTrekPhotoProvider(...args: Parameters) { - return photos.setProvider(...args); -} - -export function deleteTrekPhotoIfOrphan(photoId: number): void { - photos.deleteIfOrphan(photoId); -} diff --git a/server/src/nest/reservations/reservations.bridge.ts b/server/src/nest/reservations/reservations.bridge.ts deleted file mode 100644 index 78c84ce98..000000000 --- a/server/src/nest/reservations/reservations.bridge.ts +++ /dev/null @@ -1,74 +0,0 @@ -import { db } from '../../db/database'; -import { DatabaseService } from '../database/database.service'; -import { RealtimeService } from '../realtime/realtime.service'; -import { ReservationsService } from './reservations.service'; -import { PermissionsService } from '../permissions/permissions.service'; -import { BudgetService } from '../budget/budget.service'; -import { ExchangeRatesService } from '../budget/exchange-rates.service'; -import { notificationsInstance } from '../notifications/notifications.instance'; - -export type { EndpointInput, ReservationEndpoint, ReservationTraveler } from './reservations.service'; - -/** - * Non-Nest entry point for the reservations domain — for code running OUTSIDE - * the Nest container (the legacy tripService, the airtrail import/sync - * services, and the still-legacy transports MCP registrar; the reservation - * MCP tools and resource moved to the DI-discovered reservations.mcp.ts, the - * transit tools to the DI-discovered transit.mcp.ts (which injects - * ReservationsService), and the plugin RPC host injects ReservationsService - * via ReservationsRpc). Exports only the legacy services/reservationService - * names still consumed outside the container, 1:1, so repointing a consumer is - * an import-path-only diff. Inside the container, inject ReservationsService - * instead. Delete exports here as their consumers migrate. - * - * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton. - */ -const reservations = new ReservationsService( - new DatabaseService(db), - new PermissionsService(new DatabaseService(db)), - new BudgetService(new DatabaseService(db), new PermissionsService(new DatabaseService(db)), new ExchangeRatesService(), new RealtimeService()), - new RealtimeService(), - notificationsInstance(), -); - -export function listReservations(tripId: string | number) { - return reservations.list(tripId); -} - -export function loadEndpointsByTrip(tripId: string | number) { - return reservations.loadEndpointsByTrip(tripId); -} - -export function resyncReservationDays(tripId: string | number): void { - reservations.resyncReservationDays(tripId); -} - -export function createReservation(tripId: string | number, data: Parameters[1]) { - return reservations.create(tripId, data); -} - -export function getReservation(id: string | number, tripId: string | number) { - return reservations.getReservation(id, tripId); -} - -export function getReservationWithJoins(id: string | number) { - return reservations.getReservationWithJoins(id); -} - -export function updateReservation( - id: string | number, - tripId: string | number, - data: Parameters[2], - current: Parameters[3] -) { - return reservations.update(id, tripId, data, current); -} - -export function deleteReservation(id: string | number, tripId: string | number) { - return reservations.remove(id, tripId); -} - -export function notifyBookingChange(tripId: string | number, actorId: number, booking: string, type: string): void { - reservations.notifyBookingChange(tripId, actorId, booking, type); -} diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 6e54f5a71..33cd14788 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -123,10 +123,10 @@ type AccommodationTimesMeta = { * legacy call paths. The legacy route's budget side effects (auto-create / * update / delete a linked budget item) and the booking notification are * encapsulated here so the controller stays thin — behaviour is 1:1. - * Non-Nest consumers (legacy tripService, airtrail import/sync, the transit + - * transports MCP registrars) go through reservations.bridge.ts instead of - * importing this class directly; the plugin RPC host injects it via - * ReservationsRpc. + * Every consumer injects this class now. reservations.bridge.ts existed for + * the legacy tripService, airtrail import/sync and the transit/transports MCP + * registrars, all of which have folded in; the plugin RPC host reaches it + * through ReservationsRpc. */ @Injectable() export class ReservationsService { diff --git a/server/tests/integration/memories-synology.test.ts b/server/tests/integration/memories-synology.test.ts index bfb3a2efd..436a0f826 100644 --- a/server/tests/integration/memories-synology.test.ts +++ b/server/tests/integration/memories-synology.test.ts @@ -1186,7 +1186,15 @@ describe('Synology SSRF blocked error handling', () => { // ── Passphrase persistence fixes ───────────────────────────────────────────── -import { getOrCreateTrekPhoto, deleteTrekPhotoIfOrphan } from '../../src/nest/photos/photos.bridge'; +import { TrekPhotosRepository } from '../../src/nest/photos/trek-photos.repository'; +import { DatabaseService } from '../../src/nest/database/database.service'; +import { db as trekDb } from '../../src/db/database'; + +// Was photos.bridge, which existed for consumers outside the container and had +// none left. The repository is what it delegated to. +const trekPhotos = new TrekPhotosRepository(new DatabaseService(trekDb)); +const getOrCreateTrekPhoto = (...a: Parameters) => trekPhotos.getOrCreate(...a); +const deleteTrekPhotoIfOrphan = (id: number) => trekPhotos.deleteIfOrphan(id); import { decrypt_api_key } from '../../src/nest/common/crypto/apiKeyCrypto'; describe('trek_photos passphrase healing (SYNO-090)', () => { diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index b3b100a83..66e39f618 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -56,7 +56,10 @@ import { createUser, createTrip, createDay, createPlace, createDayAssignment, cr import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { DaysService, DayReorderError, addDays } from '../../../src/nest/days/days.service'; -import { getDay as bridgeGetDay, listDays as bridgeListDays } from '../../../src/nest/days/days.bridge'; +// Was days.bridge, deleted with the other three that had no consumer outside the +// container. The assertions stayed; they point at the service now. +const bridgeGetDay = (id: string | number, tripId: string | number) => svc.getDay(id, tripId); +const bridgeListDays = (tripId: string | number) => svc.list(tripId); import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; @@ -252,7 +255,7 @@ describe('remove', () => { // consumer (legacy tripService) folded into the DI-native TripsService — // 029/031/032 pin the same behavior on the service. -describe('days.bridge', () => { +describe('DaysService — the surface the deleted bridge exposed', () => { it('DAY-SVC-027 — getDay delegates to DaysService.getDay', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index 837809db8..88131ba1a 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -63,7 +63,9 @@ import { createUser, createAdmin, createTrip, addTripMember } from '../../helper import { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { PackingService } from '../../../src/nest/packing/packing.service'; -import { listItems as bridgeListItems } from '../../../src/nest/packing/packing.bridge'; +// Was packing.bridge, deleted with the other three that had no consumer outside the +// container. The assertions stayed; they point at the service now. +const bridgeListItems = (tripId: string | number, viewerId?: number) => svc.listItems(tripId, viewerId); import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; @@ -773,7 +775,7 @@ describe('notifyTagged', () => { // ── Bridge delegation ───────────────────────────────────────────────────────── -describe('packing.bridge', () => { +describe('PackingService — the surface the deleted bridge exposed', () => { it('PACK-SVC-050: listItems delegates to PackingService over the shared db Proxy', () => { const { user: owner } = createUser(testDb); const { user: other } = createUser(testDb); diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index 15c77e97a..f63d33db8 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -2,7 +2,7 @@ * Unit tests for the DI-native ReservationsService — RESV-SVC-001+ (real-SQL * cases over the folded legacy services/reservationService.ts statements), * RES-TRAV-001..005 (moved 1:1 from tests/unit/services/reservationTravelers.test.ts) - * and RESV-BRIDGE-001..009 (one delegation case per reservations.bridge export). + * and RESV-BRIDGE-001..009, which kept their assertions when the bridge went. * Uses a real in-memory SQLite DB so the SQL logic — falsy-coercion defaults, * COALESCE update semantics, day derivation, the accommodation/budget cascades * and the TEXT accommodation_id normalization — is exercised faithfully. @@ -57,7 +57,26 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; import type { BudgetService } from '../../../src/nest/budget/budget.service'; -import * as bridge from '../../../src/nest/reservations/reservations.bridge'; +// Was reservations.bridge, deleted along with the other three that had no +// consumer outside the container. The cases below kept their assertions and +// point at the service the bridge delegated to; only the delegation itself is +// gone, and there was nothing left to delegate for. +const bridge = { + listReservations: (tripId: string | number) => svc.list(tripId), + loadEndpointsByTrip: (tripId: string | number) => svc.loadEndpointsByTrip(tripId), + resyncReservationDays: (tripId: string | number) => svc.resyncReservationDays(tripId), + createReservation: (tripId: string | number, data: Parameters[1]) => svc.create(tripId, data), + getReservation: (id: string | number, tripId: string | number) => svc.getReservation(id, tripId), + getReservationWithJoins: (id: string | number) => svc.getReservationWithJoins(id), + updateReservation: ( + id: string | number, + tripId: string | number, + data: Parameters[2], + current: Parameters[3], + ) => svc.update(id, tripId, data, current), + deleteReservation: (id: string | number, tripId: string | number) => svc.remove(id, tripId), + notifyBookingChange: (...a: Parameters) => svc.notifyBookingChange(...a), +}; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; @@ -513,7 +532,7 @@ describe('setReservationTravelers / loadTravelers (#1517)', () => { }); }); -describe('reservations.bridge (legacy-named delegation for outside-container consumers)', () => { +describe('ReservationsService — the surface the deleted bridge exposed', () => { it('RESV-BRIDGE-001: listReservations delegates to ReservationsService.list', () => { const { trip } = ownerTrip(); createReservation(testDb, trip.id, { title: 'A' }); diff --git a/server/tests/unit/services/trekPhotoMedia.test.ts b/server/tests/unit/services/trekPhotoMedia.test.ts index 1ac6b187f..f1efdd4e9 100644 --- a/server/tests/unit/services/trekPhotoMedia.test.ts +++ b/server/tests/unit/services/trekPhotoMedia.test.ts @@ -23,7 +23,16 @@ vi.mock('../../../src/config', () => ({ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { createUser } from '../../helpers/factories'; -import { getOrCreateLocalTrekPhoto, getOrCreateTrekPhoto, resolveTrekPhoto } from '../../../src/nest/photos/photos.bridge'; +import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { db as trekDb } from '../../../src/db/database'; + +// Was photos.bridge, deleted with the other three that had no consumer outside +// the container. These call the repository directly now. +const trekPhotos = new TrekPhotosRepository(new DatabaseService(trekDb)); +const getOrCreateTrekPhoto = (...a: Parameters) => trekPhotos.getOrCreate(...a); +const getOrCreateLocalTrekPhoto = (...a: Parameters) => trekPhotos.getOrCreateLocal(...a); +const resolveTrekPhoto = (id: number) => trekPhotos.resolve(id); beforeAll(() => { createTables(testDb); diff --git a/server/vitest.config.ts b/server/vitest.config.ts index f6d759f4f..64d80aa49 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -125,6 +125,26 @@ export default defineConfig({ 'src/nest/vacay/**/*.ts': { statements: 82, branches: 66, functions: 90, lines: 86 }, 'src/nest/weather/**/*.ts': { statements: 93, branches: 78, functions: 91, lines: 97 }, 'src/nest/**/*.ts': { statements: 80, branches: 80, functions: 80, lines: 80 }, + + // Outside the container. Coverage MEASURES src/**, but until now only + // src/nest/** was gated, so the boot path, the scheduler, the MCP + // handler and the migrations sat under no floor at all — which is + // exactly where the worst finding of the last audit lived (a swallowed + // EADDRINUSE that no test could have caught). These entries pin what is + // measured today so it cannot slide; they are floors to raise, not + // targets that have been met. + // + // src/demo/** is deliberately absent: it measures 0%, and a floor of + // zero asserts nothing. It needs tests before it needs a threshold. + 'src/app-config/**/*.ts': { statements: 99, branches: 93, functions: 99, lines: 99 }, + 'src/db/**/*.ts': { statements: 73, branches: 38, functions: 59, lines: 80 }, + 'src/mcp/**/*.ts': { statements: 58, branches: 43, functions: 63, lines: 60 }, + 'src/middleware/**/*.ts': { statements: 91, branches: 89, functions: 87, lines: 94 }, + 'src/systemNotices/**/*.ts': { statements: 84, branches: 67, functions: 93, lines: 85 }, + 'src/utils/**/*.ts': { statements: 92, branches: 87, functions: 89, lines: 96 }, + // index.ts, bootstrap.ts, scheduler.ts, config.ts, websocket.ts and the + // small top-level modules beside them. + 'src/*.ts': { statements: 39, branches: 36, functions: 36, lines: 42 }, }, }, },