Requested in discussion #1658: an expense carries an amount and a
category, and everything else about it ends up crammed into the title.
The column was already there. It just was not available, because the
costs UI stores an itemized receipt in it as TICKETJSON:{...}, so an
expense split by receipt could never carry a note - and saving one any
other way wrote note=null, which quietly wiped whatever had been typed
into the budget table. The receipt gets its own column and note goes
back to being text. Old clients that still send the blob as a note are
routed to the right column instead of clobbering the note.
On desktop the row now has fixed columns, so the amount does not drift
left when one row has a note and the next does not. The category moved
to the card edge as a tab like the mobile card does, which is what frees
the line the note sits on. Collapsed it is one line, and the same
element grows in place when opened.
The dialog is two columns now: what the expense was on the left, how it
is shared on the right, grouped into panels the way the trip dialog
does it.
On the phone the eleven category pills became a dropdown, which is where
the room for the note field came from, and a note on a card unfolds when
you tap it.
A place that has shut down for good is never the answer to "where should
we go", so it should not be in the results at all.
Google knows this and says so in businessStatus, we just never asked for
the field. It goes into the field mask and CLOSED_PERMANENTLY drops out.
CLOSED_TEMPORARILY stays - a restaurant on holiday next month is still
worth planning around - and results without the field are parks and
viewpoints rather than businesses, so they stay too.
OSM states it a few different ways. Re-tagged objects (disused:amenity,
abandoned:shop) never matched the selectors to begin with, but plenty
keep their original tag and gain a marker instead, and those did come
back. Same for opening_hours=closed/off.
Roadmap census 14 -> 8 (5 seam-pinned, 3 verified-permanent), resources.ts DoD line closed, README in-container list and counts corrected. Includes the SystemNoticesService unit test the src/nest coverage ratchet demanded after the addonEnabled threading.
Both advisories are the same mistake: a route that hands out a bearer token
treated trip membership as authorisation. Membership lets someone read the trip
while they are signed in. It does not let them mint a URL that reads it without
an account, keeps working after they are removed, and can be rotated out from
under the owner's calendar subscriptions.
The calendar feed token now sits behind TripAccessGuard plus
@RequirePermission('share_manage'), the same shape the rest of the trip-scoped
controllers use, which also replaces a hand-written access check that GET never
called at all. GET is gated with the writes because it returns the credential
itself, not its status. The share link's GET does what its own POST and DELETE
already did, and what the invite link next door already does for exactly this
reason.
Two things had to come with it or the gate would have been decoration. The MCP
tool get_share_link read the same token on membership alone, so it would simply
have become the new way in. And feed_token rides along in every trip payload
because TRIP_SELECT reads t.*: any member could have read the value out of
GET /api/trips/:id, the summary tool or the plugin RPC. It is blanked once in
the select and stripped in the two places that select the row directly.
The three service writes take the acting user and scope their UPDATE to a trip
that user can reach. The guard decides who may manage the credential; this only
makes sure a caller arriving another way cannot write to an id it guessed.
Client side, "Subscribe to calendar" is now behind the same permission on
desktop and mobile. Without that a member would have kept an entry whose enable
button silently does nothing, because IcsSubscribeModal swallows every non-200.
The one-off ICS download stays where it is: that is a file they may already read.
Instances that want members to manage these links can lower share_manage to
trip_member in the admin panel, which the tests cover.
A transport also carries a top-level reservation_time, and that branch derives its zone from the linked place, which a flight does not have, so both ends landed as floating times in the subscribed feed (#1453). Consult a timed departure endpoint first instead and keep the IANA zone per side. When the arrival side has no clock of its own, the DTEND still comes from reservation_end_time, so a rental imported with only a geocoded pickup keeps its multi-day return.
The admin panel's test button reported a key with an HTTP referrer restriction as invalid, because the validation call was the one Google request that did not send a Referer. It sends the same header googleFetch already sets for every real request.
typecheck:tests landed with the contract ratchet; the scheduler migration
merged after it, and its idempotency-cleanup test stubs register() with no
parameters. Reading calls[0][2] back off a no-parameter mock types every
recorded call as [], which is a tuple index error, so dev has been red since.
Give the stub the real register() signature and drop the cast it needed.
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.
An adversarial audit over the three merged migration PRs. Sixteen findings
survived triple refutation; these are the ones with a defect behind them.
The serious one is mine, from the gateway move. ws registers
`error: this.emit.bind(this, 'error')` on the http server it attaches to, and
the adapter now runs inside buildApp, before listen(). So from that moment the
http server HAS an error listener and Node stops throwing on one. The adapter's
handler was `() => {}`, which meant EADDRINUSE was forwarded there and dropped:
the process survived, the listen callback never ran, so the banner never
printed and the scheduler never started. A container that looks healthy and
serves nothing. Reproduced, then fixed at both ends: index.ts owns the fatal
decision on the http server, and the adapter logs instead of swallowing.
Four DTOs were tightening contracts rather than describing them, which is the
thing the pipe-before-handler ordering makes easy to do by accident:
- OauthClientCreateDto replaced 'Name is required' with a Zod field report, and
pre-empted the handler's 403 on an instance with the MCP addon off.
- BookingImportConfirmDto demanded per-item fields the service never reads, so
a caller assembling items itself got a 400 about `source`.
- TagCreateDto/TagUpdateDto typed name and color as strings; the legacy route
bound whatever arrived, so a numeric name used to create a tag.
- PluginUserSettingsUpdateDto typed config as a record, where the handler
answers 200 for anything it cannot use.
Two of my own tests were false-green, which the audit demonstrated by mutation
rather than by reading. WSGW-008 asserted on frames that never carry the user
object at all, so it passed with the password_version strip deleted; it reads
the retained identity now. GEO-009 made the same assertion before and after the
write; it distinguishes a cached negative from a miss now, through a hasCached
seam that says what getCached cannot. Both were re-run against the mutation
they exist to catch, and both fail on it.
booking-import geocodes every venue and endpoint in one request loop, up to
thirty sequential calls. Folding maps onto the shared Nominatim cursor put that
loop on the interactive lane, so an import starved every concurrent keystroke
for half a minute. It takes the background lane now, which is what the two-lane
split was built for.
Also: the README's in-container bridge list had outlived one entry and was
missing another, and the journey e2e still mocked a module this branch deleted.
The plugin host's job scheduler now uses the cron package (the one
@nestjs/schedule wraps), so node-cron and @types/node-cron leave the
dependency tree entirely. Verified against both real libraries that the
accepted cron grammar is identical — no plugin's declared schedule tightens —
and pinned that with an unmocked compat test.
The transport was a hand-rolled setupWebSocket(server) that index.ts kicked
off with a dynamic import after listen(). It is a @WebSocketGateway now, with
DatabaseService and EphemeralTokenService injected instead of imported, and
the heartbeat owned by a lifecycle hook rather than an interval nobody stops.
The wire protocol does not move. Nest's WsAdapter dispatches on
{ event, data } and every deployed client sends { type, tripId }, so
TrekWsAdapter translates rather than asking clients to change. It also builds
the ws server itself instead of delegating: the base's port-0 branch uses
noServer and performs the upgrade from its own registry, which never consults
verifyClient, so delegating would have dropped the origin check and answered
101 to anyone. The per-socket error listener stays attached first, because an
unhandled one kills the process (#1576), and the flood guard counts every
inbound frame before parsing, because malformed frames are exactly the ones
worth limiting.
Four things had to stay where they were, and each is a silent failure if it
does not:
The socket registry stays module state. Seventeen bridges and
notifications.instance build `new RealtimeService()` outside the container;
rooms on a provider instance would leave their broadcasts going to an empty
map, with no error and no log.
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,
which shows up as drag-and-drop that jumps rather than as an error.
buildApp creates the http server and binds the adapter before app.init(), and
callers take it from getHttpServer(). Nest binds gateways during init, so a
later adapter binds nothing; an adapter given the app binds to Nest's own
internal server, which this process never listens on. Both boot green with no
client able to connect.
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). Eighteen e2e harnesses
died that way before the split.
src/websocket.ts survives as a re-export of the three fan-out primitives. 115
test files vi.mock that path to assert on broadcasts.
The two transport suites pass unchanged, and 30 new cases pin the pieces
directly: the pv and MFA gates, the numeric socket id, the plugin announcement
firing before the room check, both sides of the origin allowlist, and the
flood guard counting frames no handler wants.
The trip/todo reminder enable gates (notify_trip_reminder, notify_todo_due)
are now read inside each 9 AM tick instead of once at boot, so toggling them
takes effect at the next run without a restart. Saving notification settings
no longer re-arms anything: the auth-side startTripReminders() call and the
shouldRestartScheduler result field are gone (no controller consumed it, so
the PUT /api/auth/app-settings response shape is unchanged). The boot
'enabled/disabled' log lines are informational banners now.
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.
Client CI was red on this branch while dev was green, and no client file had
changed. The 536 test files all passed; the job failed on three unhandled
rejections, all `window is not defined` from MDashWidgets, attributed to
whichever case happened to be running when they landed.
The widget fetches api.frankfurter.dev directly and sets state from both
handlers with no unmount guard. MSW runs with onUnhandledRequest: 'warn', so
that request was never intercepted: it left the runner, came back after the
environment was gone, and React tried to schedule an update against a torn
down tree. Load dependent by construction, which is why dev happened to pass.
Two halves, because either alone leaves the other broken. The widget now
aborts in flight work on unmount and on refetch, and treats an abort as "no
one is waiting" rather than as a failure to report. That matters outside the
runner too: this is a third party call on a mobile screen a user can leave
immediately.
The handler set gains the third party endpoints the app calls directly rather
than through /api, so no suite reaches the real host. An empty rate list is
the shape the widget already handles.
Also clears twenty more test files, each at its root cause. Two more were
passing for the wrong reason: oidc.service.test.ts built AuthService with an
AtlasService in the membership slot, undefined for userCleanup and mailer,
and the mailer stub in the tokens slot, the same shift admin.service.test.ts
had. And CAL-010 claims to serialise a flight byte for byte, but
createReservation ignores reservation_time, so the row is written with a null
time and CalendarService drops it: the recorded snapshot is truthful about
the data the case builds and silent about the flight it advertises.
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.
conditions.test.ts lost noTrips when the condition kind landed; oauth.test.ts
lost the RFC 8707 resource indicator when it became required. Both are
fixtures that stopped matching a contract that grew, and both are filled in
with the value that keeps every existing case reading as it did.
budget.service.calc.test.ts typed its prepare mock from the default
implementation rather than from prepare's own signature, so all ten later
mockImplementation calls had to reproduce that exact three-member literal.
One stmt() helper names the widening instead.
admin.service.ts reached getOnlineUserIds with require('../../websocket')
inside a try/catch — the last lazy require of the ws singleton in the tree,
and exactly what RealtimeService exists to prevent. The facade gains the
third pass-through next to broadcast and broadcastToUser, delegating at call
time like its siblings so per-file vi.mock stubs keep flowing through.
The catch stays at the admin call site rather than moving into the facade: an
admin list that shows everyone offline is a better answer than a 500, but
that judgement belongs to this caller, not to every future one.
Typechecking the tests turned up two more things behind the arity errors.
admin.service.test.ts built AuthService with an AtlasService in the
membership slot and no mailer at all, so its last four collaborators were
shifted by one. Nothing failed, because no case there reaches a path that
uses them.
Its getOidcSettings/updateOidcSettings wrappers referenced methods AdminService
no longer has. Nothing called them: OIDC settings moved to OidcService and the
two cases moved with it (ADMIN-SVC-047/048 now live in oidc.service.test.ts).
Only the wrappers stayed behind.
server/tsconfig.json has include: ["src"], so tests/ was never typechecked.
A service that gained a constructor parameter did not break its hand-wired
call sites at compile time — the argument arrived as undefined and threw at
runtime, if anything read it at all. The notifications sweep hit 46 such
sites and only a stash-diff against a throwaway tsconfig found them.
tsconfig.tests.json makes that permanent. It is not wired into CI yet: the
tree still reports 170 errors, and turning it on now would block every
unrelated PR.
Three of the first fixes were not type noise.
DAY-SVC-034 asserted that createAccommodation rolls back a failed
reservation insert. That method moved to AccommodationsService in the
days/accommodations split, so the case was calling undefined — which throws,
which satisfied .toThrow(). The rollback has been unchecked ever since,
while the case reported green. Repointed at the owner; the invariant holds.
host-rpc.test.ts typed its response as RpcResponse & Partial<RpcError>.
Both carry ok, one as true and one as false, so the intersection is never
and all 97 property reads in the file were errors nobody could see.
auth.controller.test.ts passes {} as the body in cases that stub the service
and assert on the mapping around it. That is correct as written — the point
of several of them is that the error path never reads the body — so anyBody()
names the intent instead of inventing fields the assertions do not read.
Three changePassword calls were genuinely missing the @Res() argument.
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.
streamImmichAsset answered { error, status } when the effective owner had no
Immich connection stored, and left the response untouched. All four callers
ignored that return, so the client got no status line and waited out its own
timeout. Its Synology counterpart has always written through
handleServiceResult; this is the same shape.
Every other exit already went through pipeAsset, which writes on all paths
including its catch, so the method is void all the way down now — and the
PhotoProvider interface drops the error return it was carrying purely to
describe this defect.
Three cases pin it, including the one that matters for shared albums: the
connection that gets read is the OWNER's, so a viewer who happens to have
Immich configured must not paper over an owner who does not.
Dispatch to a photo backend was `switch (photo.provider)` in
photo-resolver.service.ts twice and, in journey-public.controller.ts, an
if/else that handed anything not named immich to Synology — where it 404'd
from inside that provider's id parser. Adding a third backend meant finding
all three.
PhotoProviderRegistry.get(id) replaces them, over adapters registered under
the PHOTO_PROVIDERS token in memories.module.ts. Adapters rather than
`implements` on the services themselves: ImmichService and SynologyService
are also the album and browse surfaces, and folding the interface into them
would drag their argument orders and return shapes along.
PhotoAssetRef is the part worth having. The two services took the same three
values in different orders — Immich as (userId, assetId, ownerId), Synology
as (userId, targetUserId, photoId) — so every dispatch site had to remember
which was which, and a transposition would have compiled and served another
user's photo. One place builds the ref, one place decrypts the Synology
share passphrase.
`local` stays an explicit branch at both dispatch sites: the bytes are on
this disk, there is no row for it in photo_providers, and its asset info
comes from the stored row rather than from a backend.
The rename memories/ -> media/ is deliberately not here. It is import churn
across 275 files, and coverage-thresholds.mjs buckets on one path segment,
so merging photos/ in would collapse two ratchet entries into one and keep
the looser of the two.
Nine *.mcp.ts files reached through addons.bridge for one boolean each, and
platform.routes plus the legacy registrars still do. That was never a
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, and the only thing in scope was a module-level singleton that
builds its own AddonsService outside the container. Nine domains hid an edge
to the addons domain from the module graph to answer "is this addon on".
@trek/nest-mcp hands the gate its declaring instance now, resolved at
attach rather than captured at module load, and addons/addon-gate.ts turns
that into addonGate(ADDON_IDS.X) over the injected service. The option is
declared with method syntax so a predicate written against its own
controller class stays assignable.
Two more edges went the same way, both checked for cycles first:
reservations.mcp injects AssignmentsService (AssignmentsModule never
reaches back), atlas.mcp and journey.mcp inject AuthService.
The tests that forced a toggle by mocking the bridge module now write the
addon row the admin panel writes, so they exercise the real read.
Four in-container uses stay, each documented in the nest README: places.mcp
(a real Days -> Places -> Assignments cycle), budget/packing/costs against
trips.bridge, user-cleanup against budget.bridge, and the two multer
interceptor options, whose fix is MulterModule.registerAsync.
Atlas throttled to the 1.1s OSM asks for and cached its answers in a 50k
map; maps did neither, so five interactive routes fired straight at the
service while the two atlas backfill loops politely waited their turn.
From Nominatim's side that is one instance ignoring the rate limit.
Both go through geo/nominatim.client.ts now. Three things the naive merge
would have got wrong:
- The shared client sends UA, the instance-identifying User-Agent, not the
generic string atlas used. A shared generic UA is throttled harder.
- One cursor across five interactive routes and two background loops would
have put a keystroke behind however many places the backfill enqueued, so
there are two lanes over the one cursor and background yields.
- The cache key carries the query shape. Atlas asks at zoom 3 and 8, maps at
18; a coordinate-only key answers a street lookup with a country.
The throttle cursor and the cache stay module state, the way the permissions
and FX caches do, because the bridges build their collaborators outside the
container and a second copy of the cursor is the bug being removed here.
GeocodingService owns only the cleanup timer's lifecycle, which atlas used to
start as an import-time side effect.
Also folds the three haversine implementations into common/geo.ts, keeping
the clamped form: asin of a value a hair over 1 is NaN, and a NaN distance
fails every comparison it feeds instead of throwing.
tests/setup.ts zeroes the interval. Nineteen maps cases drive these paths
back to back with real timers and were sleeping nineteen seconds for it.
Two controllers declared the identical @Controller string from two modules and
each rebuilt the trip access check its own way: booking-import ran it as a
guard chain, integrations ran it by hand inside the handler, and
BookingImportService carried a third copy that nothing called at all.
They never actually collided - the four file-upload sub-paths and the one
AirTrail sub-path are disjoint - so this is not a bug fix. It is one access
path instead of three implementations of the same one, and the dead copy is
gone.
The addon requirement sits on the AirTrail handler rather than the class.
Class-level would have 404d the four upload routes whenever AirTrail happened
to be off. AddonGuard still runs first in the chain, because a disabled addon
has to answer 404 to an anonymous caller before anything answers 401, and
ADDON-GUARD-006 pins both halves of that.
One deliberate behaviour change: the AirTrail route now gets its trip 404 from
the guard, ahead of the body 400, because a guard runs before the pipe. That
is the ordering every other trip-scoped controller already has and nothing
pinned the other way for this route.
The class rename would have stopped the server booting: the body-contract gate
keys on ControllerClass.methodName and throws on stale entries. Three entries
renamed, and the list re-sorted - GATE-006 caught that the rename had broken
the ordering it asserts.
booking-import lost the controller from its coverage denominator, so the
branch floor was about to fall. It goes up instead: the off-request parse had
no case for its catch arm, so a throw would have left the job on 'running' and
the widget spinning, and the binary probe behind /api/health/features had no
test at all - that one boolean decides whether the client offers booking
import.
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.
Every split on this branch dropped the giving domain's ratchet without a line
being tested any worse: coverage follows the source file, so when
well-covered code leaves, the weaker branches that stay behind become the
whole denominator. days fell from 82 to 77.5 percent branches that way, feeds
to 76.7, trip-read-model to 62.5.
The rule in vitest.config.ts is that the only way past a number is to raise
it, so these are tests, not edits to the thresholds. days is back at 92.1,
calendar at 96.2, trip-read-model at 100.
feeds and trip-members and trip-read-model had no unit test at all - feeds'
whole threshold rested on one e2e, which is exactly the file whose contract
this branch rewrote. They have one now, and the read-model test pins the
thing in that file that would leak data if it regressed: getTripSummary and
bundle both thread the viewer id into packing.listItems so another member's
private items stay hidden, and nothing asserted that before.
The five thresholds this branch introduced move up to what they now measure,
with a few points of slack. The pre-existing entries are left alone on
purpose: they were generated from a Linux run, CI measures there, and a
handful of branches are platform-specific - raising them from a Windows
measurement is how you get a failure that reproduces nowhere local.
The Express shell set a 100kb limit and stopped applying it the moment the
Nest instance took over parsing: bootstrap passed bodyParser:false and
nothing replaced it. The number happened to stay the same, because it is also
the framework default - but it lived nowhere anybody would look for it, and
one NestFactory default away from silently becoming something else.
It goes through Nest's own parser rather than a second express.json(),
because that parser carries the verify hook that keeps req.rawBody, and the
plugin webhook routes need the exact payload bytes to check a provider's
HMAC. Adding a parser in front of it would have taken rawBody away without
failing anything.
MISC-009 and MISC-010 pin it from both sides.
The bodyParser flag on applyGlobalMiddleware goes with it: one caller, which
passed false.
Not moved: cookieParser stays on the raw Express layer rather than becoming a
configure(consumer) middleware. The platform routes - the OAuth consent flow
among them - are registered on that instance before Nest routing exists, so a
Nest-slot cookie parser would simply not run for them. helmet, CORS,
compression, the forced-HTTPS 301 and the res.on('finish') logging stay for
the same reason: they have to see requests Nest never routes.
The last live non-shell middleware verified the JWT and read the users row a
second time on every /api request, then threw the loaded user away - two
jwt.verify calls and two SELECTs per authenticated request, because it ran
before Nest and could not see what the auth guard had already resolved. The
guard reads req.user.
It also corrects an asymmetry that had grown quietly. The middleware decided
on two literal path lists, and every public /api endpoint added after those
lists were written - /api/config, /api/help/*, the public journey and share
routes, /api/health/features - answered a logged-in user without MFA with a
403 while answering the same request from a stranger perfectly well. Nothing
intended that; the lists simply stopped being maintained. Keying on @Public()
means the exemption cannot drift from the route again. This is a deliberate
behaviour change and the only one here.
@MfaExempt carries the reason for the other kind of exemption: the routes
that let somebody complete setup. Without them the policy locks out every new
account, which is why the middleware had its second list.
One ordering trap on the way: global guards run before route guards, so the
auth guard standing down on a controller with its own @UseGuards chain left
req.user unset when the MFA guard ran behind it, and it waved everything
through. AUTH-020 caught it. It resolves the user now without refusing - the
declared chain still owns the 401.
Nothing in src/nest/** was an APP_GUARD. Every protected route said so
itself, 62 times, which means a forgotten @UseGuards was a silent bypass
rather than an error. GlobalAuthGuard inverts that: a route is authenticated
unless it carries @Public() or @OptionalAuth(), or declares its own guard
chain.
Reading the declared chain is what makes this behaviour-neutral. Nest runs
global guards before route guards, so without that check the global guard
would 401 an anonymous request to an addon-gated controller before its
AddonGuard could answer 404 - inverting an order four e2e suites pin, and
leaking which addons are installed. A controller that declared a chain keeps
deciding for itself.
It is a new class rather than JwtAuthGuard gaining a Reflector: that guard is
constructed positionally in its own test with a context stub that has no
getHandler, and the two want opposite behaviour anyway.
@Public() takes a required reason, and the reasons are the diff worth
reading: the invite token IS the credential, help assets are loaded by <img>
which cannot send one, logout has to work with an expired session or nobody
can sign out.
The real deliverable is the inventory. validateRouteGuards runs in buildApp
next to the body-contract gate and refuses to start when a route answers
anonymously without being on a reviewed list of 25 - or when a listed one no
longer does. An allow list nobody prunes stops being a list.
Default-deny found two controllers on its own: SharedController and
OidcController both sit in a file next to a guarded controller, so a
file-level search for unguarded ones missed them. 37 tests went red and named
them. That is the mechanism working before the change shipped, not after.
src/nest/** had no owner guard at all. Handing a trip over and creating or
deleting guests each asserted access.user_id !== user.id inline, and the
string they answer with differs per route, so the message travels in the
metadata rather than living in the guard.
TripOwnerGuard 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 let any admin
transfer somebody else's trip. OWNER-004 pins that: an admin who is not the
owner gets 403.
places gets protection on the five routes where a guard is provably safe:
list, get, unrate, image and delete. Not on create, update, the imports or
the uploads. A guard runs before the pipe, so guarding a route with a
DTO-typed body would answer the trip 404 where the suite pins a 400 - that
includes PUT :id/rating, which is why the rating pair is split. The three
multipart routes are excluded for the other reason: answering while the
client is still streaming destroys the socket, and the caller sees
ECONNRESET instead of a status.
DELETE :id carries @RequirePermission('place_edit'). The first attempt at
this lost that check and answered 200 where 403 belonged, so the e2e asserts
the argument list rather than the status: a check that returns the right code
for the wrong reason fails now.
TripsService injected fourteen other services. Almost all of them were there
for two read aggregates - the MCP trip summary and the offline bundle - and
for the member roster, not for anything the write path does. Reading is what
fans out; writing is not.
Membership goes to trip-members/: listMembers, addMember, removeMember,
transferOwnership and the four guest methods, plus the six routes, which keep
their paths. It is a NEW module, not the trip-membership/ leaf next door:
deleting a guest erases their plugin data and re-splits their expenses, so it
needs auth and budget, and AuthModule already imports that leaf. Giving it
those dependencies there would have closed a hard cycle. This one is a sink -
nothing imports it back except trips.
The read aggregates go to trip-read-model/, which is where the nine-service
fan-out now lives.
What is left is the write core plus day generation, on eight parameters
instead of fourteen: database, reservations, days, permissions, budget,
vacay, realtime, unsplash. The last four are all reachable from updateTrip's
date-shift transaction, which is why the count does not fall to three.
Trips still reaches budget through ReservationsModule, so trips.bridge.ts
survives this. Saying otherwise would have been the easy claim to make and it
would not have been true.
The routes lived in reservations/ while the SQL lived on DaysService, so the
controller delegated every call across a domain boundary to reach its own
data, and neither module owned the thing outright. The seven statements move
into AccommodationsService unchanged and the domain gets its own directory,
module, DTO, RPC and MCP surface.
What stays in days/ is the reorder side: assertNoInvertedAccommodation and
resyncAccommodationDays are about re-dating day rows, and the stay is the
thing being carried, not the thing doing the carrying.
Two edges fall out of this. AccommodationsService drops its DaysService
dependency, and DaysMcp drops DatabaseService and PlacesService, because
create_place_accommodation was their only user - which lets DaysModule stop
importing PlacesModule and closes one leg of the latent
Days -> Places -> Assignments -> Days cycle.
DAY-SVC-016..026, 029 and 035 move to accommodations.service.test.ts with
their case ids intact. The reservations e2e container assembles both modules
now: it builds a single domain module on purpose, which is exactly what
catches a move like this, and it caught it.
days/ carried two fachlichkeiten with a full file set each: days.controller,
days.service, days.mcp, days.rpc, days.dto next to the five day-notes ones.
The note files move out unchanged; because the new directory sits at the same
depth, not one import path inside them changed.
The two modules deliberately do not reference each other in either direction:
dayExists is raw SQL on the days table in the note service, and DaysService
reads day_notes the same way, so a module edge would buy nothing and cost a
cycle.
DayNotesModule has to be registered in PluginsRuntimeModule as well as
AppModule - DayNotesRpc owns four of the 113 wire methods and the registry
boots with requireTotalCoverage, so leaving it out would stop every test app
that assembles the plugin runtime.
The days e2e container assembles both modules now: the note routes still nest
under the days prefix, and a container that builds only DaysModule would 404
on them while production served them fine.
The second flake recorded as 'costs' is not a timing flake. It is a live
HTTPS call: budget.service.ts settlement() reaches getRates()
unconditionally, unlike the two guarded call sites above it, and
ExchangeRatesService fetches api.frankfurter.dev with a 10 s abort.
tests/integration/budget.test.ts stubbed fetch nowhere at all, so the suite
talked to the internet - fast when the API answers, ten seconds per call
when it does not. The stub fails closed, which changes no assertion: every
settlement case here is single-currency, so rates never enter the
arithmetic. tools-budget-advanced had the stub inside a single case; hoisted
so a second settlement case cannot quietly reintroduce the call.
The five ReDoS budgets in maps.service.test.ts measured wall-clock time,
which on a loaded machine reports how long the test waited for a core.
MAPS-024 has gone red at 523 ms against an unchanged file for exactly that
reason. They now measure CPU time, which is what catastrophic backtracking
actually burns. This does not soften them: CPU time is always <= wall-clock
time, so every budget got stricter, and pool:'forks' keeps the measurement
to this file.
jubnl