Zooming into England showed nothing below "England" itself, so boroughs
marked as visited had no polygon to light up. It stopped working in 3.1.0,
when the runtime Natural Earth fetch was replaced by the bundled geoBoundaries
assets: Natural Earth carried 232 GB regions including the London boroughs,
and geoBoundaries calls the four constituent countries ADM1. The builder asks
every country for ADM1, so GB shipped as four features.
GB now ships at ADM2 — 216 counties and unitary authorities, which is the
granularity every other country gets from its ADM1 and the granularity that
was there before. That is a deliberate one-country list: ADM2 everywhere
would multiply the bundle for no gain, since ADM1 already is the level people
think in for most countries.
Two things had to be said to data that already exists. Regions marked before
3.1.0 hold the old Natural Earth codes, and are reconciled by name within the
country, the way the 3.1.1 step already does for everywhere else; a row that
still cannot be matched is left alone rather than destroyed. And place_regions
is full of GB-ENG rows that were correct under the old bundle — nothing
re-derives a row that is already cached, so without clearing them every
English place would keep reporting "England" forever.
The bundle guard that would have caught this in 3.1.0 is now in place: four
features for a whole country is not a region layer.
Closes#1974
A packing item is Common, Personal or Shared with named recipients. listItems()
always applied that rule, but every path that touches one item resolved it by
trip id alone, so trip membership plus packing_edit was enough to reach an item
the member could not see. Update, delete, clone, the contributor routes and the
sharing route all shared the lookup, and the same service methods sit behind the
MCP tools and the plugin RPC, so all three transports carried the gap.
The update was a read as well: it answered with the enriched row, which hands
back the name, category, quantity, weight, the owner's username and the recipient
list of an item the caller was never shown.
Two flows laundered restricted items into places they do not belong. Saving a
trip as a template read every row in the trip, so a template captured the names
and categories of everyone's Personal items. Copying a trip did the same and
re-inserted without is_private/owner_id, so both fell back to the column defaults
and another member's Personal or Shared item reappeared in the copy as a Common
item. That path needs only trip_create and trip access, not even packing_edit.
There is now one visibility rule in the service and every single-item lookup goes
through it. The actor is threaded through the HTTP, MCP and plugin-RPC entry
points, and a missing actor denies rather than falling through unfiltered. An
item the actor may not see is reported as missing, not as forbidden, so the
routes cannot be used to confirm that an id exists.
Two deliberate contract changes fall out of that. The sharing route answers 404
instead of 403 when the caller cannot see the item at all; a visible item they do
not own still answers 403. And a template or a trip copy now carries the Common
list plus the actor's own items, with a restricted item staying restricted and
owned by the copier.
Reported privately by Chichi (@vickyzer027-hash).
AirTrail 3.12.0 renamed the flight's passenger list from `seats` to
`passengers` and moved flightReason from the flight onto each passenger. There
is no alias in either direction: a 3.12.0 instance returns `passengers` and
`null` for `seats`, and its save schema accepts only `passengers`.
TREK read `raw.seats` everywhere, so against a 3.12.0 instance it found nothing.
The seat never made it into the import, and because a pull rebuilds the metadata
from the AirTrail payload, flights that had already been imported lost the seat
they had. That is the report. Flights whose sync is switched off were never
re-pulled, which is why theirs survived.
Two more breaks came from the same rename and were not reported. The flight
reason stopped importing for the same reason. And the writeback pushed the seat
under `seats`, which the new schema strips, so a seat edited in TREK was quietly
dropped on the way out; it did not fail loudly because the spread of the fetched
flight carried AirTrail's own `passengers` through untouched.
Both shapes are handled rather than switched between, because instances on 3.11
and older are still out there: reads take `passengers ?? seats`, and the save
body carries the manifest under both keys. Neither schema is strict, so each
version keeps the key it knows and strips the other.
The snapshot hash deliberately keeps feeding the manifest in under the same key.
An unchanged flight on an older instance therefore hashes exactly as before and
does not re-sync, while a 3.12.0 flight hashes differently once and pulls the
missing seat back in. There is a pinned-hash test guarding the first half.
Verified against real instances rather than the docs: a 3.11.1 and a 3.12.0
container, seat imported from each, a seat edited in TREK written back and read
out of AirTrail again, and the pre-fix build failing the same checks on 3.12.0.
Closes#1931
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.
BODY_CONTRACT_ALLOW_LIST is empty. The last seventeen handlers type their
body with a createZodDto class, so the fail-closed boot gate has nothing left
to forgive; it still refuses a boot for a new handler that reads @Body()
without one.
Several of those contracts describe rather than constrain, and that is the
whole care in this change. The pipe runs before the handler, so a schema
stricter than the handler moves a user-visible answer:
- TagsController.create answers { error: 'Tag name is required' }, pinned
byte-for-byte in two tests, and the pipe formats as `field: message`. The
DTO leaves name optional and the handler keeps the check that owns the
message. Same shape for the booking-import confirm body, which additionally
gains per-item validation the handler never did.
- PluginsController.activate and install answer 503 when plugins are disabled,
and check that BEFORE looking at the body. A schema that rejected first
would turn that into a 400 for an operator whose server has plugins off.
- setEgressHosts reads a non-array `hosts` as "clear the list", which is the
documented reset. Typing it as string[] would have removed a working admin
action rather than tightened anything.
- PluginRoutesController.route answers { route: null } at 200 for everything
it dislikes, so the client falls back to straight lines. A 400 there would
turn a graceful degradation into a visible error.
files.bridge is gone. The multer options are built by
MulterModule.registerAsync from an AllowedFileTypesService leaf, so the
fileFilter reads the operator's extension list from the container instead of
from a second FilesService built outside it. Nest injects
MULTER_MODULE_OPTIONS with @Optional(), so a token that fails to resolve is
not a boot error: it silently falls back to defaults, with no filter and no
size cap. FILE-020 already proved that for trip files by uploading an .svg
over HTTP and expecting 400; journey uploads had no such case, and now do.
VIDEO_UPLOAD is untouched, including the filename callback that forces .jpg
regardless of what the client sends, which guards the public share proxy.
PluginFrame gets an onError. A frame whose document fails to load left a
blank element at full height, which reads as a hung app rather than as a
plugin that is gone.
typecheck:tests runs in CI now, with the test tree at zero errors. tz-lookup
is left as it is: the build puts it in a lazy helpers chunk, not the entry
chunk, and the remaining split would trade a synchronous render for an async
one to move 172 kB that is already off the critical path.
ephemeral-tokens.ts was free functions, and AuthService owned the cleanup
timer only because it happened to be the domain that mints tokens. Issuing
them and sweeping them are different jobs, and FilesService and TokenService
mint or consume them too.
EphemeralTokenService owns the lifecycle and hands the three operations out.
The store and the interval stay module state next door, and that is
load-bearing rather than a leftover: src/websocket.ts consumes ws tokens on
the upgrade handshake, outside the container, before any injector is in
reach. Per-instance state would hand it an empty Map and close every
connection with 4001, which no test would catch because the ws suites drive
the transport directly. WS-027 through WS-029 pin the session gate that runs
through exactly that path.
The MAX_STORE_SIZE back-pressure is the second reason: createEphemeralToken
returns null at 10,000 live tokens and TokenService turns that into a 503. A
second store would raise the real ceiling with nobody noticing.
Its own leaf module rather than a provider in AuthModule: TokensModule mints
download and ws tokens as well, AuthModule already imports TokensModule, and
TokensModule is documented as a deliberate leaf that three graphs reach for.
Same shape as MailerModule, which broke AuthModule <-> NotificationsModule.
Twelve hand-wired construction sites in tests needed the new collaborator.
The typecheck named all twelve; before this branch they would have been
undefined at runtime.
Sixteen more test files are cleared here too, each at its root cause. Four of
them were passing for the wrong reason:
files.controller.test.ts drove two handlers with shifted arguments, a string
tripId where the trip row belongs and undefined where tripId and id belong.
It stayed green because FilesService is fully stubbed and the stubs ignore
what they are given, so DELETE /:id/permanent and DELETE /trash/empty never
covered that the trip row reaches the permission check.
conflictUpdate.test.ts was missing real collaborators on PlacesService and
PackingService. None of the exercised paths reach them, so it passed by luck.
assignments.service.test.ts builds its service by hand, and the one consumer
of the collaborator it was missing swallows every throw in a bare catch. A
broken journey reconciliation would still have shown 28 green tests.
scheduler.test.ts mocks two modules with paths that resolve one directory
above the server root, so they mock nothing at all. Harmless only because
src/scheduler.ts imports neither.
Nine fire-and-forget sends reached notifications.bridge through
import('...').then(({ send }) => ...). The laziness was working around a
module cycle that no longer exists, and it cost twice: it hid the edge from
the module graph, and it handed each send a NotificationsService built
outside the container rather than the container's own.
The sweep needed a method rather than a grep. tests/ sits outside
tsconfig's include, so a service that gains a constructor parameter does not
break its hand-wired call sites at compile time — the argument arrives as
undefined and throws inside the send. Adding tests to a throwaway tsconfig
and diffing the errors before and after named all 46 sites exactly. The same
config reports 288 pre-existing errors in the suites, almost all deliberate
partial constructions in the harnesses, which is why tests/ is not in the
build's include; that cleanup is its own piece of work and is written down
in the nest README.
notifications.instance.ts holds the out-of-container instance, apart from
notifications.bridge.ts on purpose: several suites replace the bridge
wholesale with vi.mock(..., () => ({ send })), and a sibling bridge taking
its instance from there would break under those mocks.
The suites that assert on a send now hand their spy to the injected service.
The reservations e2e overrides the provider, because its app resolves the
real one out of its own container and never saw the module mock.
src/services/ held one thing: airtrail/, 1305 lines of plain functions over the
better-sqlite3 singleton. They are four providers and two pure helper modules
now, and the directory is empty.
Five hidden edges go with them. The db singleton in three files, the raw
broadcast from src/websocket in two, and the reservations, addons and audit
bridges - all module-level imports that the module graph could not see.
The scheduler stops reaching for it at call time. scheduler.ts ran
require('./services/airtrail/airtrailSync') inside the cron tick, so a wiring
mistake surfaced as "AirTrail sync tick failed" every five minutes rather than
at boot. It takes the dependency through SchedulerDeps now, the same structural
contract backups and placePhotos already used.
One edge stays a seam, deliberately. AirtrailSyncService needs
ReservationsService, because the inbound pull applies remote changes through
the real reservation update path - endpoint restamping and all - and
reimplementing that against raw SQL would be worse than the coupling. The
write-back trigger runs the other way, from reservations.controller, and
injecting there would close
ReservationsModule -> AirtrailModule -> ReservationsModule. airtrail.bridge.ts
holds that one function with the reason in its docblock, rather than a
forwardRef hiding it.
The write-gate suite was module mocks over free functions and is stubs through
a constructor now; it gained four cases on the way (auth failure, remote
delete, disconnected owner). The domain ratchet goes from 19/0/10/19 - set when
integrations was two thin controllers - to 69/52/72/71, which is what the
folded code measures. It is a 1:1 move, so it arrives with the coverage its
tests always gave it.
src/services/backupService.ts moves to src/nest/backup/backup.impl.ts, imported
only by its own domain now that the controller reaches it through BackupService.
src/services/ is down to airtrail/.
It stays a module of free functions rather than becoming methods on the service.
The restore path closes and reinitializes the core DB handle, which is the single
most dangerous sequence in the server, and rewriting its shape in the same step
as the move would make a regression there impossible to bisect. The db-lifecycle
question is the last step of this fold, not part of it.
The move also pulled 512 lines out of ungated src/services/ into a gated domain,
which is how the per-domain ratchet caught that the per-entry extraction loop had
no tests at all: the zip-slip refusal, the running decompressed-byte cap (the
declared size in the central directory is attacker-controlled, so only the
running total catches a lying archive), a corrupt stream not being dressed up as
a size refusal, and the reopen failure that has to report restart-required rather
than success. Six cases now cover it, and backup goes from 84% to 96% statements.
journeyService (1324 LOC, 36 exports) and journeyShareService (203) become
JourneyDomainService and JourneyShareService. Every SQL statement, error string
and broadcast is the one that shipped.
The two could never be split: the share service gates on isOwner, which lives in
the domain service, so it injects it.
JourneyDomainModule is a leaf carrying just the two services. Places,
assignments and the plugin host each need a handful of journey functions;
importing JourneyModule for that would drag MemoriesModule and both photo
providers into their graphs. Same shape as TrekPhotosModule.
The 23 tools in src/mcp/tools/journey.ts become JourneyMcp. src/mcp/tools/ is
down to _shared.ts and registerTools() no longer fans out to anything.
journey.bridge.ts keeps four exports, for the journey resources in
src/mcp/resources.ts that register at import time.
McpAccessMode is host-augmentable now. journey:share is neither read nor write,
so the three share tools would have had to be opaque predicates - and predicates
bypass the access policy entirely, invisible to the boot-time scope gate. A
scope typo in one would ship as a silently ungated tool. McpAccessModeRegistry
mirrors McpAccessGroupRegistry; unaugmented, mode stays 'read' | 'write'.
Fixes a latent bug in tests/helpers/mcp-test-controllers.ts: it constructed
PlacesService with five of eight arguments. tsconfig only includes src, so
nothing typechecked it, and the missing dependencies were harmless until the
journey skeleton hooks landed on the place write paths.
journey.mcp.ts arrived from src/mcp/ at 14.5% branch coverage - the legacy
registrar had no test file of its own. 40 tool tests take it to 93.4%, and 25
cases for the skeleton lifecycle, the photo gallery and the preference row take
the domain service from 73.9% to 82.8%.
The seven files in services/memories/ become injectable services. Every SQL
statement, error string, status code and header moves verbatim; what changes is
who owns what.
photoResolverService was two files in one, so it stayed two: the trek_photos
rows are a repository under photos/, the provider dispatch is
PhotoResolverService under memories/. That is the photos/ vs memories/ boundary —
storage there, dispatch here — and it is also why the module graph is a straight
line: TrekPhotosModule is a leaf both halves import, so neither needs a
forwardRef.
pipeAsset and the ServiceResult envelope stay free functions over the Express
response. pipeAsset is deliberately not a StreamableFile: it forwards the
upstream 206 and Content-Range so a <video> can seek (#823), swaps Cache-Control
to no-store on an error status, and answers differently once headers are sent.
The trek-photo cache keeps its in-flight map at module scope. That map is the
stampede guard, and the scheduler builds its own instance through a lazy require
outside the container — a per-instance map would give them separate guards and
ten tiles would fetch the same asset ten times, silently.
journeyService's dynamic import() of synologyService is gone: it existed only to
duck the import cycle, which no longer exists.
MemoriesModule imports RealtimeModule explicitly even though it is @Global, so a
single-domain e2e TestingModule can still resolve the broadcast.
The providers arrived at 60% branch coverage or less, because services/memories/
sits outside the gate and the integration suites drive the domain over HTTP
without reaching the failure paths. New suites cover what had no case at all:
the access checks that decide who may fetch someone else's photo, the stampede
guard, the thumbnail mtime check, the poster-less-video 404, and Immich end to
end with safeFetch stubbed — including the hidden-asset filter older servers
depend on (#1474) and the http -> https canonical detection that must NOT
rewrite a changed host.
photoResolverService was two files in one. One half registers and drops rows in
trek_photos and touches no photo provider at all; the other dispatches a stream
or an info lookup to immich or synology. Keeping them together meant every
consumer that only wanted to store a row - the unified trip-photo service, the
journey service - dragged both providers in behind it.
The storage half becomes TrekPhotosRepository in nest/photos, over
DatabaseService. That is also where the photos/ vs memories/ line belongs:
storage here, provider dispatch with memories. photos.bridge.ts serves the two
modules still in services/ that register rows, and dies with them.
Every statement moved verbatim, including the two rules worth stating out loud:
getOrCreate re-encrypts a passphrase onto an existing row rather than inserting
a second one, and deleteIfOrphan never deletes a local photo, because its bytes
are ours and the file would outlive the row.
Four files taken apart rather than moved, because they only worked as a knot.
The mailer becomes its own leaf module. AuthService sends the password-reset
mail while NotificationsModule imports AuthModule for the demo gate in
NotificationsMcp - a mailer inside the notifications domain would have closed a
hard cycle that only a forwardRef could open again. Webhook and ntfy stay with
notifications; auth has no business importing them.
The channel registry stays a module singleton and does NOT become a provider.
PluginRuntimeService pushes its channel getter in at onModuleInit, and
notifications.bridge.ts builds its own NotificationsService for the scheduler
crons and the memories senders. A provider would hand the bridge a second, empty
registry - every plugin channel would then go quiet with no error anywhere.
CHOVR-015 drives that seam end to end: a channel published through the runtime,
delivered through the bridge. Nothing pinned it before; the existing cases all
set the source themselves.
The built-ins no longer self-register by side-effect import. They used to exist
only because notificationPreferencesService happened to `import
'./notifications/builtins'`; removing that line would have silenced email,
webhook and ntfy without a failure. They are built from the injected transports
and registered from NotificationsService's constructor, so both instances have
them.
No cached nodemailer transport: MailerService.createTransport re-reads the SMTP
config and skipTlsVerify per send, exactly as the three inline call sites did,
so an admin changing them does not have to restart.
Preferences become a provider over DatabaseService. Eight places built
NotificationsService by hand, so a tests/helpers/notifications.ts factory keeps
the next constructor parameter a one-file diff instead of an eight-file one.
* refactor(server): move the webauthn config and the account erasure into nest/auth
webauthnConfig resolved the RP ID through its own getSetting() on the db
singleton; it is a WebauthnConfigService over DatabaseService now, injected into
AuthService (passkey_configured) and PasskeyService (four ceremonies). readEnv()
is still read per call, so an operator flipping the RP ID at runtime takes effect
on the next ceremony instead of after a restart.
userCleanupService follows as UserCleanupService. It keeps taking
removeUserFromBudgetItems from budget.bridge rather than injecting BudgetService:
BudgetModule imports AuthModule for the demo guard in BudgetMcp, so AuthModule
cannot import BudgetModule back, and breaking that edge the other way drags
BudgetController, BudgetMcp and its trips.bridge chain into every graph that
imports AuthModule.
The legacy module had no tests of its own - the erasure only ever ran through
AdminService.deleteUser and TripsService.deleteGuest - so USER-CLEANUP-001..009
pin both halves, including the orphan data dir that no permissions row can
describe and the all-or-nothing deletion transaction.
* refactor(server): fold the oauth 2.1 server out of services/
694 lines move into OauthService over DatabaseService, AddonsService and
AuditService. The thin wrapper that only forwarded is gone, and so is the second
path to the MCP addon flag - validateAuthorizeRequest read it through
addons.bridge while the wrapper read it through the injected service, which would
have let the two drift.
The pending authorization codes stay a module singleton in oauth.pending-codes.ts
instead of becoming instance state. The SDK still serves /oauth/authorize,
/oauth/register and /oauth/consent from its own router, mounted on Express before
app.init(), so it reaches the domain through the new oauth.bridge.ts - two maps
would break the authorization-code flow silently, since the SDK only ever answers
"Authorization grant is invalid.". The import-time sweep keeps its .unref() and is
a named function now so a test can drive it.
scopes and sessionManager are imported deep, never through the src/mcp barrel:
the barrel re-exports revokeUserSessionsForClient and pulls the domain bridges
back in, which is the nest -> mcp -> nest cycle. Same comment as admin.service and
auth.service carry.
The 70 cases of the legacy suite moved over unchanged, bound to a hand-built
service. Fifteen more cover what free functions never let anyone reach: the code
store at capacity, an expired code, an expired access token, refresh against a
public and an unknown client, the RFC 8707 resource checks, and that a code
written through the container is read back through the bridge.
* refactor(server): move the wiki reader into the help module
wikiService had exactly one consumer, HelpController, and touches no database -
it stays a plain module rather than becoming a provider, the way
auth/ephemeral-tokens.ts does, so the text and asset caches keep being one per
process without any ceremony around it.
The WIKI_DIR anchor needed one more level: src/services sits three below the repo
root, src/nest/help sits four. The existing path-resolution case caught it, which
is what it is there for - a wrong anchor makes help degrade to the GitHub fallback
without anybody noticing.
Also brings the nest README back in line: services/ is down to five top-level
files, and the passkey and budget entries no longer point at paths that were
deleted since they were written.
Two small leftovers, moved to the domains that own them.
ephemeralTokens holds the short-lived tokens the WebSocket handshake and the
file-download links use. It already had startTokenCleanup/stopTokenCleanup with
an unref-ed handle — the right shape. What was missing is anyone calling stop:
index.ts kicked the sweep off with a bare require() after listen() and nothing
ever ended it. AuthService owns that lifecycle now, through
onModuleInit/onModuleDestroy, which is where it belongs — it is the service that
issues the tokens.
The store itself stays module state. websocket.ts consumes tokens from outside
the container, and a token store is only useful if the issuer and the consumer
share one. Same reasoning as the weather cache in #1835.
inAppNotificationActions is a 22-line handler registry with exactly one
consumer; it moves next to it as notifications/in-app-actions.ts.
services/ is down to 9 files, from 44.
Finishes be-weather-airports-fold. The README has listed weather as the
reference implementation since the migration started, while 513 lines of it sat
in the legacy layer.
The file is nest/weather/weather.impl.ts now, and WeatherService owns what it
could not own before: the cache sweep. That sweep was a bare setInterval at
module scope, so every process that so much as imported the file started a
five-minute timer and nothing ever stopped it — there was nothing to call. It is
startCacheCleanup/stopCacheCleanup now, driven by onModuleInit/onModuleDestroy,
and the handle is unref-ed so a cache sweep can never hold the process open.
The old test file says as much: its second line is "Prevent the module-level
setInterval from running during tests", with a vi.useFakeTimers() to do it. That
workaround is no longer what keeps the timer quiet.
The cache itself stays module state, deliberately. The MCP weather tools call
getWeather directly from outside the container, and a cache is only worth having
if every caller sees the same one. Hoisting it into the class would mean handing
the registrar the container instance to get exactly what a module singleton
already provides — the same trade the place-photo cache resolved the other way,
because there the scheduler needed the instance anyway.
Coverage is the reason this PR carries eleven new tests rather than none. Code
in services/ is ungated; the moment it lands in src/nest it counts against the
80% branch threshold, and weather.impl arrived at 75%. The gap was in paths
worth having: a failing archive lookup with and without an upstream reason, a
far-future date whose climate rows are all null, an expired entry being re-
fetched, and the sweep evicting one on its own. Plus the lifecycle itself —
start is idempotent, stop is idempotent, the handle is unref-ed. 79% now.
services/ is down to 11 files, from 44.
The last file in be-places-helpers-fold, and the one that had to be a provider
rather than a move: it carries three in-memory structures that only do their job
if the process has exactly one of them.
recentFailures stops a screenful of markers hammering an API that is down
inFlight stops a stampede when several requests miss the same placeId
knownOnDisk saves an existsSync per cache hit
A second instance gives the stampede guard its own inFlight map, so two
simultaneous requests for the same uncached place both call the provider. That
is why the scheduler takes this one from the container instead of constructing
it: SchedulerDeps gains a placePhotos entry alongside backups, filled from
app.get() in index.ts — the shape PR #1827 was built for.
The module-scope mkdirSync becomes onModuleInit, and GOOGLE_PHOTO_DIR is read
per call through RuntimeEnvService rather than frozen at import. That is not
cosmetic: TREK_PLACE_PHOTO_DIR is the seam the tests use to keep off the real
uploads tree, and its own test used to have to import the module dynamically to
set the variable in time. The __dirname fallback moved a level with the file and
is corrected accordingly.
Consumers: maps, places and share inject it; places.helpers.reclaimPhotoCache
takes it as a parameter, since it is a free function.
Six test seams follow. Four path mocks become injected stubs with the same mock
functions behind them; the collections case keeps the REAL implementation,
because what it asserts is that removeIfUnreferenced counts collection_places as
a reference (#1081) — a stub there would assert nothing. The share e2e overrides
the provider in its container.
services/ is down to 14 top-level files, from 44 when this started.
Two of the three files in be-places-helpers-fold. placePhotoCache is left for
its own PR — it holds three module-level caches and its sweep hangs off the
cron, so it needs both a provider with real state and a second entry in the
scheduler contract.
kmlImport.ts is 212 lines with no state, no database and no I/O — parsing,
sanitising and coordinate arithmetic. It is nest/places/kml-import.helpers.ts
now, unchanged, next to the places.helpers.ts that already consumes it.
placeImage.ts moves to nest/places/place-image.ts as free functions. Making
reclaimPlaceImage a provider would have split it from PLACE_IMAGES_DIR, which
cannot be injected: nest/common/place-image-upload.ts reads it at module scope
to configure multer's storage, before any container exists.
That constant is the one thing here that could have broken quietly:
path.resolve(__dirname, '../../uploads/places')
__dirname moves with the file. From src/services that resolved to
<server>/uploads/places; from src/nest/places the same string would have
resolved to <server>/src/uploads/places — a directory multer would have happily
created, leaving every uploaded place image in the wrong place and every
existing one unreachable. The path is now '../../../uploads/places', verified to
resolve to the identical absolute directory, and uploads-dirs.test.ts covers it.
Two of the three leftovers in be-misc-fold. The third (wikiService) needs a Zod
contract in @trek/shared first and is left for its own PR.
llmConfig.ts was pure movement: 70 lines of encryption-aware config shaping with
no state and no database, sitting in services/ while all four of its callers are
in src/nest. It is nest/llm-parse/llm-config.ts now, same functions, same file.
unsplashService.ts needed an actual provider. Its key resolution reads both the
env and the users table (env var, then the caller's stored key, then any
admin's), so the injected connection was unavoidable. Its own module because
trips and places both reach it, and because folding it into either would have
made the other import a trip/place service for an image search.
The four exports became methods on UnsplashService. The SSRF guard in
saveUnsplashCover is untouched — it still refuses any host but the Unsplash
image CDN and still goes through safeFetch — as is the unauthenticated-endpoint
fallback when no key resolves (#1449).
The env read stays live through RuntimeEnvService rather than a registerAs
token. readEnv() is what the function did before, and a boot-frozen token would
change when an operator's UNSPLASH_ACCESS_KEY takes effect.
Two test seams follow the code: the unsplash unit tests bind the four methods so
every existing assertion reads unchanged, and the trips controller's path mock of
the module becomes an injected stub.
services/ is down to 17 top-level files.
* refactor(server): route every trip-access check through DatabaseService
Twelve Nest services carry the same verifyTripAccess/canEdit pair. Six already
asked DatabaseService.canAccessTrip; the other six went through
services/tripAccess, a nine-line re-export of the identical function. Same
query, same result, two ways to reach it.
All twelve now take the injected route, so services/tripAccess is gone.
The bigger duplication was in the types. Thirteen files declared their own
`Trip` for the same return value, in three different shapes:
NonNullable<ReturnType<typeof verifyTripAccess>> 6x
{ user_id: number } 4x
TripAccess 3x
The narrow one is why assignments, days, places and accommodations all wrote
`as Trip | null | undefined` on the call — the cast existed to shrink
TripAccess down to a local type that was never meant to differ from it. All
thirteen now say TripAccess, and the four casts are gone with no other change.
middleware/tripAccess.ts goes too: it was already dead in production — nothing
under src/ imported requireTripAccess or requireTripOwner, only comments in the
services that had absorbed them. Its test went with it.
* refactor(server): fold the query helpers and trip joins into the container
Two more legacy files whose only reason for staying outside the container was
that they predated it.
services/queryHelpers.ts held five exports doing two different jobs. Three are
the batch loaders that keep the list endpoints off N+1 queries — they hit the
database and are now QueryHelpersService over the injected connection, imported
by assignments, days, places and share. The other two, formatAssignmentWithPlace
and ratingAggregate, touch no database at all; they sat next to the loaders only
because the loaders were free functions too. Those stay free functions, in
common/rowShape.ts.
The README note on the assignments service claimed the loaders had to stay in
services/ because the day, share and place services were unmigrated. All three
have since moved, so the reason is gone.
services/tripMembership.ts becomes TripMembershipService in its own module. It
could have folded into auth or trip-invite, but both call it — as do oidc and
the plugin RPC host — and putting it in either would have made AuthModule and
TripInviteModule import each other.
Neither module is @Global, matching PermissionsModule: e2e TestingModules
resolve them transitively through each consumer's explicit import.
Test seams changed shape accordingly. Two path mocks of services/tripMembership
became injected stubs; the trip-invite e2e overrides the provider in the
container instead. The oidc test keeps the real implementation, which is what
OIDC-SVC-045 was always asserting.
ratingAggregate moved out of the ungated services/ tree into the coverage-gated
one, so it gets the direct tests it never had — as does the tags/participants
pass-through, which two long-dead fixture variables in the moved test file had
been waiting for.
Also drops four stale comments that outlived their subject: the requireTripAccess
middleware they pointed at was deleted in the previous commit.
Eight files in services/ hold no state and touch no database: avatarUrl,
conflictResult, demo, passwordPolicy, timezoneService, cookie, and the two
crypto helpers. They are imported from 65 places, 62 of them already inside
src/nest, so having them live in the legacy layer was pure inertia.
They stay plain functions rather than becoming providers. Three importers sit
outside the container and would have no way to reach an injectable one:
db/migrations.ts runs before the app is built, and middleware/auth.ts and
middleware/mfaPolicy.ts run beside it. Wrapping these in DI would buy ceremony
and nothing else.
mfaCrypto and apiKeyCrypto move together into common/crypto — they read the
same ENCRYPTION_KEY, and the directory says so.
services/ drops from 44 files to 36, 9,870 lines to 8,493.
One trap worth recording: src/app-config and src/nest/app-config both exist.
cookie.ts imported '../app-config' from services/, which resolved to the former;
after the move the same string silently pointed at the latter, which has no
readEnv. It is now explicit about which one it wants.
Nothing else changes — no signatures, no behaviour, no tests rewritten beyond
their import paths.
The 851-line services/adminService folds into the wrapper AdminService over
DatabaseService plus injected Settings/Addons/Passkey/Packing/Auth/Permissions/
Notifications services; the auth, notifications and permissions bridge imports
all become injections, PERMISSION_ACTIONS stays a plain const, and the
mcp/sessionManager deep import keeps its anti-cycle comment. Ahead of the fold
the 11 packing-template functions move to PackingService, which already owned all
three template tables — resolving the admin-2 residual with no bridge. The pure
and module-scoped half moves to admin.helpers.ts, the version cache staying
module-scoped so admin.bridge's instance and the container singleton share one
GitHub fetch. A 1-export admin.bridge.ts serves scheduler.ts's cron, the only
out-of-container consumer. Recipe steps 3 and 4 are no-ops: no src/mcp/tools/
admin.ts has ever existed and the plugin host never imported the domain.
Four lines are non-verbatim, all path re-anchoring one directory deeper, both
resolved paths verified against the emitted dist layout.
admin.dto.ts clears all twenty AdminController allow-list entries, retiring the
three 'enabled must be a boolean' checks plus 'permissions object required' and
'Object body required' for the pipe envelope. The module e2e goes DI-native: the
3-method whole-module mock dies, 6 cases become 15 over real SQL.
Quirk fixes on top, each pinned: the three places toggles go fail-closed with an
append-only backfill migration; the template item routes honour :templateId;
updateUser rejects an empty username/email; createInvite 404s an unresolvable
trip_id; listOAuthSessions guards its scopes parse; GitHub fetches gain a timeout
and size cap and failures cache briefly; updateOidcSettings/updateAddon/updateUser
become transactional; admin.oauth_session.revoke is renamed to match its
siblings; two mutations gain their missing audit rows; getAuditLog drops the
_parse_error sentinel.
Tests move with IDs preserved (ADMIN-SVC-001..069 incl. the pre-existing 029/030
gap and duplicated 069, VNOTIF-001..007), plus ADMIN-BR-001 and ADMIN-SVC-070..076.
The send() dispatcher (services/notificationService.ts) and the in-app store
(services/inAppNotifications.ts) fold together into the DI-native
NotificationsService over DatabaseService + RealtimeService; the prefs
matrix, transports, channel registry and inAppNotificationActions stay plain
infra modules. A 1-export notifications.bridge covers scheduler, legacy
adminService/memories and the six lazy fire-and-forget sends; AdminController
and the plugin RPC host (24th factory dep) inject. The 5-tool registrar +
notifications-in-app resource move to notifications.mcp.ts. Tests move with
case IDs preserved (NSVC-*, INOTIF-*, + NSVC-020 bridge pin); ~18 suites
repoint path mocks/warm-ups; the module e2e goes DI-native.
Also carries the verified-quirk fixes that would normally trail as a
separate fix(server) commit (file-overlap with the fold): toUtcIso on all
three created_at paths (INOTIF-013/014), respond claims the response CAS
before running the action handler with release-on-failure (INOTIF-011/012),
synology's fire-and-forget send gains .catch, the dispatch-failure log
unwraps Error messages (NSVC-021), and the MCP resource description drops
its false 'unread first' claim.
Fold the 1497-line services/authService.ts into the DI-native nest/auth
domain (Wave-5 chain opener, unblocked by the atlas fold):
- Pure crypto half (backup-code hash/match/generate, stripUserForClient,
key masking, the import-time DUMMY_PASSWORD_HASH timing equaliser and
avatarDir mkdir — documented parity exceptions) moves verbatim to the
plain module nest/auth/auth.helpers.ts.
- DB half folds into AuthService over DatabaseService + injected
PermissionsService (ex permissions.bridge) + AtlasService (ex
atlas.bridge, deleted on schedule). mfaSetupPending and the per-email
reset throttle stay module-scoped so the bridge and container instances
share them; avatarDir and require('../../../package.json') re-anchor one
directory deeper — with the two injection swaps, the only non-verbatim
lines. Controller-facing method surface unchanged.
- New 8-export auth.bridge.ts for out-of-container consumers: mcp/index.ts
token verification, the journey/notifications/transports registrars'
isDemoUser, and the still-legacy adminService/oidcService/passkeyService.
- 15 domain *.mcp.ts demo guards now inject AuthService (their modules
import AuthModule); atlas.mcp.ts alone stays on the bridge because
AuthService injects AtlasService and the reverse module edge would close
an AuthModule<->AtlasModule cycle (places.mcp precedent). OidcService and
PasskeyEnabledGuard inject AuthService too.
- Tests move with case IDs preserved: authService.test.ts ->
auth.helpers.test.ts, authServiceDb.test.ts -> auth.service.test.ts
(+ AUTH-BR-001..007 bridge delegation, + AUTH-DB-050..088 pinning the
previously untested getAppConfig/login/MFA/reset/admin-settings branches
now under the src/nest coverage gate). auth.e2e converts DI-native (real
bcrypt, real audit rows); oidc.e2e swaps its dead path mock for an
instance spy; websocket/integration suites repoint to bridge/helpers.
Quirks preserved byte-for-byte (0-coord drop in getTravelStats,
un-transactioned registerUser/verifyMfaLogin multi-writes, unguarded
revokeUserSessions in deleteMcpToken, current! assertions in
updateApiKeys) — fixes land separately.
Fold the 1612-line services/atlasService.ts into the DI-native AtlasService:
the stats aggregation, visited countries/regions with the #1490 tombstone and
cascade logic, and the bucket-list CRUD become methods over the injected
DatabaseService. The ~750-line pure-geo half — the bundled admin0/admin1
boundary stores with their #1576 streaming builders, the point-in-polygon
indexes, Nominatim geocoding with its shared throttle, and every module-scoped
cache including the import-time unref'd cleanup interval — moves verbatim to
the plain module atlas-geo.ts so the 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 mcp/tools/atlas.ts registrar plus all four atlas resources in
mcp/resources.ts move onto the decorator registry as atlas.mcp.ts — the when:
atlas-addon gate is parity here (legacy tools and resources both gated; the
REST controller deliberately does not). resources.test.ts retires with its
last two cases, which move to tools-atlas-expanded.test.ts. The plugin RPC
host swaps its 9 atlas imports for the injected AtlasService (23rd constructor
dep); a minimal 2-export atlas.bridge.ts serves the one legacy consumer,
authService.getTravelStats, and dies when authService migrates.
Adopts atlas.dto.ts over the existing shared schemas and clears all three
AtlasController entries from body-contract-allow-list.ts: the hand-rolled
'name and country_code are required' 400 becomes the ZodValidationPipe
envelope (todo/places trade) while the whitespace-only bucket name keeps its
legacy 'Name is required' trim guard.
Fixes the verified defects the relocation had faithfully carried, each with a
regression test (ATLAS-SVC-031..036 plus two MCP casing cases): the four
multi-statement mark/unmark writes now run in db.transaction() (the region
cascade nests as a savepoint), the trip-less countryPlaces early return
honours manually_marked, the '|| null' bindings that dropped lat/lng 0 and
empty-string notes on bucket update become '?? null', the mutating bucket SQL
is user-scoped, and the MCP region/country-places tools uppercase their codes
to match REST.
Quirks deliberately preserved: the detached GET-path place_regions backfill
writes, getStats' two divergent return shapes, the import-time cache-cleanup
interval, getCountryFromAddress's unvalidated bare 2-letter codes, and the
whitespace-only-name silent no-op on bucket update.
Tests move with their case IDs: atlasService.test.ts becomes
unit/nest/atlas.service.test.ts (pure-geo imports from atlas-geo, DB calls
through the constructed service, plus bridge-delegation cases), the atlas e2e
converts to the DI-native pattern (real SQL against the full temp-db schema),
and the plugin-host path mock becomes a constructor stub.
Fold the 1029-line services/placeService.ts into the DI-native PlacesService:
the CRUD + ratings SQL, the GPX/KML/KMZ importers and the Google/Naver list
importers become methods over the injected DatabaseService, byte-identical
statements, COALESCE semantics and the If-Match conflict protocol (#1135).
The pure half — frozen XML parsers, the KMZ unpacker, the dedup predicates,
the Google hex-id parsers, reclaimPhotoCache — moves to places.helpers.ts
(maps.helpers precedent).
The 10-tool mcp/tools/places.ts registrar plus the trek://trips/{tripId}/places
resource move onto the decorator registry as places.mcp.ts. search_place travels
with them because its gate is places:read, not the read-only geo group, and now
injects MapsService. TripsService, DaysMcp, BookingImportService and the plugin
RPC host (PluginHostDepsFactory's 21st constructor dep) all inject PlacesService,
so the domain needs no bridge at all. The two assignments.bridge imports that
places.mcp.ts keeps are a deliberate cycle break: AssignmentsModule imports
DaysModule and DaysModule now imports PlacesModule for days.mcp.ts's place
creation, so injecting AssignmentsService there would close
DaysModule -> PlacesModule -> AssignmentsModule -> DaysModule. Same seam
reservations.mcp.ts uses, same trade trips.bridge.ts documents.
Also folds services/placeEnrichment.ts in — its DB/websocket/Maps half becomes
PlacesService methods over the injected DatabaseService/RealtimeService/
MapsService, its pure match selector joins places.helpers.ts — which retires
nest/maps/maps.bridge.ts with its last consumer.
Adopts places.dto.ts for the seven grandfathered body contracts and clears every
PlacesController entry from body-contract-allow-list.ts. Two consequences worth
knowing: the bespoke 400s 'Place name is required', 'ids must be an array of
numbers' and 'URL is required' are now the ZodValidationPipe envelope, and
because a pipe runs before the handler a malformed body 400s ahead of the
trip-access 404 it used to follow — the same trade the todo and trips migrations
took. placeBulkUpdateRequestSchema.ids drops .min(1) so the endpoint's
empty-list short-circuit stays reachable.
Fixes four defects the relocation had faithfully carried, each with a
failing-first regression test:
- create dropped lat/lng of exactly 0, so a place on the equator or the prime
meridian lost its coordinates (`x || null` cannot tell absent from zero).
- duration_minutes: 0 was unsettable — replaced by the 60-minute default on
create, and read as "absent" by COALESCE on update.
- the journey delete hook fired on unscoped ids: DELETE /:id, POST /bulk-delete
and the delete_place MCP tool could detach another trip's journey entries for
an id they then refused. All three scope first via the new
PlacesService.scopedIds, mirroring the guard PluginHostDepsFactory already had.
bulk_delete_places also moves its hook ahead of the DELETE —
journey_entries.source_place_id is ON DELETE SET NULL, so running it afterwards
left the entries as orphans.
- the place search treated % and _ as LIKE wildcards; a bare % returned the whole
trip. The three clauses now carry ESCAPE with the term escaped.
Plus hardening on the Google list fetch: the response is capped at 8 MB
(declared content-length and post-read length, the transit.service precedent)
and its JSON.parse is guarded, so a malformed provider payload produces the
existing 'Invalid list data received from Google Maps' 400 instead of throwing.
Quirks deliberately preserved: the non-COALESCE route_color (#776),
currency/transport_mode staying unclearable, importGpx returning null for a file
that parses but yields no usable geometry, update skipping the If-Match check
when the stored updated_at is null, and every string-valued `x || null`.
Tests move with their case IDs: placeService.test.ts and the wrapper suite merge
into unit/nest/places.service.test.ts, kmzUnpack into places.helpers.test.ts,
placeEnrichment into places.enrichment.test.ts. The places e2e converts to the
DI-native pattern (real SQL against temp-db DDL), tools-places.test.ts is kept
and extended so it exercises the decorator path through the test registry, and
the plugin-host and booking-import path mocks become constructor stubs.
The scheduled backup archived travel.db directly. The archiver reads its
entries during finalize(), so a WAL checkpoint writing pages back mid-stream
tore the copy — and the -wal that would make it recoverable isn't in the zip.
The manual path was moved to VACUUM INTO for exactly this reason; the
scheduler never followed. It calls the same createBackup() now, which also
gets it the encryption key and plugin data it was missing, so a scheduled
backup is finally restorable onto another install.
The pre-rotation copy in migrate-encryption.ts had the same problem: a plain
file copy of a live WAL database, and it is the operator's only rollback if a
key rotation goes wrong.
Auto-backups now exclude the re-derivable photo caches, matching manual ones.
Transitous exposes routeShortName, which many operators — German long
distance among them — do not fill. displayName carries the identifier
passengers actually see, with routeShortName as the fallback.
Opening one trip fires a 404 per place without a photo, 30 to 60 of them from
one IP in seconds. CrowdSec and fail2ban read that as scanning; the reporter
had three users banned within five minutes. A place having no photo is not an
error and no longer answers as one.
OSM places were also sent to Google as node:/way:/relation: ids, which can
only return 400 INVALID_ARGUMENT — a billed round-trip and about 1.3s per
place. getPlacePhoto already guarded coords: this way; the OSM prefixes now
get the same treatment, as does getPlaceDetailsExpanded, which had no guard.
The negative cache separates a place having no photo from a failed provider
call instead of expiring both after five minutes and replaying the storm.
Fold the legacy tripService SQL into TripsService; port the trips MCP registrar/resources/trip-summary prompt to trips.mcp.ts and the share-link tools to share.mcp.ts (predicate access for the trips:delete/trips:share/canReadTrips gates, deprecation notice via the attach ctx); adopt trips.dto.ts and clear the seven allow-list entries; delete todo/share bridges with their last consumers. Includes the trailing quirk fixes: transactional deleteTrip/deleteGuest, owner display_name in listMembers.
Nest services inject RealtimeService (call-time delegation to the
websocket module, DatabaseService pattern) instead of importing
broadcast/broadcastToUser globals. Zero behavior change; test edits
are constructor-arg additions plus RealtimeModule in e2e harness
imports only.
Imported tracks all render in the same blue: the GPX/KML importers never
assign a category, so category_color is null for every one of them and
several walks in the same area are impossible to tell apart.
Adds a nullable route_color on places, kept out of the COALESCE group in
updatePlace so an explicit null can reset a track back to inheriting its
category colour. The hex is validated in the Nest controller rather than
in the shared contract: the update body is an open record on purpose, and
tightening it would mean removing PlacesController.update from the
ratchet-only body-contract allow-list.
The importers now hand each new track a colour of its own, picked from
the palette entries the trip is not already using. That happens in the
Nest service over the injected DatabaseService — the colour assignment is
new logic, and new logic does not belong in the legacy place service (see
server/src/nest/README.md). The field itself is only threaded through the
existing update/create statements, which the proportionality rule covers.
Also fixes duplicateTrip, whose explicit column list has been silently
dropping route_geometry all along, so a duplicated trip lost its tracks.
Folds the 626-line services/reservationService.ts into the DI-native
ReservationsService — byte-identical SQL, statuses, bodies, error strings
and broadcasts; the legacy file is deleted.
- reservations.mcp.ts: the 5 legacy tools (create/update/delete_reservation,
reorder_reservations, link_hotel_accommodation) + the trip-reservations
resource move to the decorator registry (identical names, schemas,
annotations, error strings, broadcasts); the canWrite/canRead registration
gates become declarative access markers. Legacy registrar and resource
entries removed.
- reservations.bridge.ts: 9 legacy-named exports + 3 type re-exports for the
outside-container consumers (tripService, airtrail import/sync, the
still-legacy transit + transports registrars, transitItineraryService's
type-only import).
- notifyBookingChange folds to the legacy (tripId, actorId) signature; the
controller passes user.id and the plugin-host factory drops its last
plain-function reservation import for a private method over the injected
service (constructor unchanged).
- TripsService and BookingImportService inject ReservationsService instead of
importing legacy functions.
- Tests: reservations.service.test.ts rewritten as a real-SQL suite
(RESV-SVC-001..030, RES-TRAV-001..005 moved verbatim,
RESV-BRIDGE-001..009 pinning the bridge); both e2e suites converted to the
DI-native pattern; path mocks repointed to the bridge or constructor stubs;
tools-reservations kept and extended with the moved resource cases.
Quirks preserved on purpose (parity first — see the follow-up fix commit):
the raw-accommodation_id metadata-sync gate, the un-transactioned
multi-statement writes, the truthy updatePositions dayId check, the
empty-string COALESCE keeps, and the TEXT accommodation_id normalization.
Folds the legacy services/settingsService SQL into the Nest SettingsService
(byte-identical statements; BEGIN/COMMIT blocks become db.transaction()).
AdminService and ShareService inject it; llm-config.resolver becomes the
injectable LlmConfigResolver (SettingsService + DatabaseService), injected by
LlmParseService and PluginHostDepsFactory. No MCP surface, no bridge.
Whole-file commit, so this also carries the post-migration quirk fixes that
live in the same files: null serializes as '' (a stored "null" leaked out of
getDecryptedUserSetting as the literal string), bulk upserts skip the masked
sentinel (mirroring the single-upsert no-op) and return the written count.
Unit suite moved to tests/unit/nest with SET-SVC-001..026 preserved,
027..030 pin the fixes; settings/admin e2e converted to DI-native.
Fold the legacy tripInviteService SQL into the Nest TripInviteService over
the injected DatabaseService (byte-identical statements; the JS-side expiry
check and non-positive-means-no-expiry coercion preserved) and delete the
legacy module. No bridge, no MCP port and no plugin-host change needed —
nothing outside the container consumed this domain.
Also wraps the rotation's probe + write + re-select in db.transaction()
per the repo rule that multi-statement writes are never bare.
Unit suite moved to tests/unit/nest preserving TRIP-INVITE-001..006 and
adding 007 (expiry pinning); the e2e converted to the DI-native pattern
(real SQL against temp trips/trip_invite_tokens tables).
Maurice