fix(server): plan 3g close-out — transactional book first-save, skeleton reconcile and share-link create; unknown-key entry patches are no-ops again; parity and authorization pins for the journey repositories

2026-10-21 22:46:32 +00:00 · 2026-09-24 00:49:55 +02:00
parent aad7605071
commit a5665b5d2d
13 changed files with 892 additions and 320 deletions
@@ -428,8 +428,11 @@ export class JourneyEntriesRepository extends TrekRepository<JourneyEntries> {
  async listCachedCountriesForPlaceIds(placeIds: number[]): Promise<{ place_id: number; country_code: string | null }[]> {
    const out: { place_id: number; country_code: string | null }[] = [];
    for (let i = 0; i < placeIds.length; i += 400) {
+      // M4 (task-5-review.md) — `chunk` can never be empty here: the loop
+      // guard `i < placeIds.length` guarantees at least one element remains
+      // to slice, so a dead `if (!chunk.length) continue;` used to sit here
+      // (an uncovered branch the coverage gate flagged, worse than base).
      const chunk = placeIds.slice(i, i + 400);
-      if (!chunk.length) continue;
      const rows = await this.kysely<PlaceRegionsKyselyDB>()
        .selectFrom('place_regions')
        .select(['place_id', 'country_code'])
@@ -1,7 +1,4 @@
-import type { Platform } from '@mikro-orm/core';
-import type { ExpressionBuilder } from 'kysely';
 import type { JourneyEntry } from '../../types';
-import { concatKysely, unixEpochToIsoKysely } from '../dialect/sql-functions';
 import type { JourneyShareTokens } from '../entities/JourneyShareTokens.entity';
 import { TrekRepository } from './_shared/trek-repository';

@@ -168,42 +165,18 @@ interface JourneyPublicKyselyDB {
  };
 }

-/**
- * R1's SECOND `GALLERY_CHRONOLOGICAL_ORDER` consumer (`getPublicJourney`'s
- * gallery read, JS15) — Task 0's worked example (`task-0-report.md`), pasted
- * verbatim against this file's own `JourneyPublicKyselyDB` shape rather than
- * re-derived. Fixed to the `gp`/`tp` alias contract `.selectFrom('journey_photos
- * as gp').innerJoin('trek_photos as tp', ...)` produces, same as the
- * `publicStayExists`/SQLF-070 precedent: a table's alias is a property of the
- * QUERY, not of the `DB` interface, so this is not generic over an arbitrary
- * caller `DB`/`TB`.
- */
-function galleryChronologicalOrderExpr(
-  platform: Platform,
-  eb: ExpressionBuilder<JourneyPublicKyselyDB & { gp: JourneyPublicKyselyDB['journey_photos']; tp: JourneyPublicKyselyDB['trek_photos'] }, 'gp' | 'tp'>,
-) {
-  return eb.fn.coalesce(
-    eb.fn<string | null>('nullif', [eb.ref('tp.taken_at'), eb.val('')]),
-    eb
-      .selectFrom('journey_entry_photos as jep')
-      .innerJoin('journey_entries as je', 'je.id', 'jep.entry_id')
-      .select((eb2) =>
-        eb2.fn
-          .min<string | null>(
-            concatKysely(
-              platform,
-              eb2,
-              { column: 'je.entry_date' },
-              { value: 'T' },
-              { expression: eb2.fn.coalesce(eb2.fn<string | null>('nullif', [eb2.ref('je.entry_time'), eb2.val('')]), eb2.val('00:00')) },
-            ),
-          )
-          .as('min_dt'),
-      )
-      .whereRef('jep.journey_photo_id', '=', 'gp.id'),
-    unixEpochToIsoKysely(platform, eb, 'gp.created_at'),
-  );
-}
+// L2 — `galleryChronologicalOrderExpr`/`listGalleryForPublicJourney` (JS15)
+// used to be a byte-identical duplicate of `JourneyPhotosRepository`'s own
+// `galleryChronologicalOrderExpr`/`galleryRead` (same `GALLERY_COLUMNS`
+// select list, same ORDER BY, differing only in which local `DB` interface
+// the builder is typed against). `JourneyShareService.getPublicJourney` now
+// calls `JourneyPhotosRepository.galleryRead` directly instead — one gallery
+// ORDER BY builder, one gallery query, per the whole-plan review (M5/L2).
+// `JourneyPublicGalleryRow` stays exported: it is still the accurate shape
+// of what that query returns (`GalleryPhoto`, the type `galleryRead` is
+// declared against, is a narrower public-API type missing `taken_at`/`lat`/
+// `lng`/`media_type`/`duration_ms`), and tests keep using it to type the
+// public gallery response.

 /**
 * `journey_share_tokens` — public share links (Plan 3g Task 3, R4). Six of
@@ -354,27 +327,6 @@ export class JourneyShareTokensRepository extends TrekRepository<JourneyShareTok
      .execute();
  }

-  /**
-   * JS15 — `getPublicJourney`'s gallery read (R1's second
-   * `GALLERY_CHRONOLOGICAL_ORDER` site). The select list is byte-for-byte
-   * `GALLERY_SELECT` (`journey-domain.service.ts`), hand-duplicated here the
-   * same way the legacy statement hand-duplicated it (that module const is
-   * not imported by `journey-share.service.ts` at HEAD either).
-   */
-  async listGalleryForPublicJourney(journeyId: number): Promise<JourneyPublicGalleryRow[]> {
-    const platform = this.getEntityManager().getPlatform();
-    return await this.kysely<JourneyPublicKyselyDB>()
-      .selectFrom('journey_photos as gp')
-      .innerJoin('trek_photos as tp', 'tp.id', 'gp.photo_id')
-      .select([
-        'gp.id', 'gp.journey_id', 'gp.photo_id', 'gp.caption', 'gp.shared', 'gp.sort_order', 'gp.created_at',
-        'tp.provider', 'tp.asset_id', 'tp.owner_id', 'tp.file_path', 'tp.thumbnail_path', 'tp.width', 'tp.height',
-        'tp.media_type', 'tp.duration_ms', 'tp.taken_at', 'tp.lat', 'tp.lng',
-      ])
-      .where('gp.journey_id', '=', journeyId)
-      .orderBy((eb) => galleryChronologicalOrderExpr(platform, eb), 'asc')
-      .orderBy('gp.sort_order', 'asc')
-      .orderBy('gp.id', 'asc')
-      .execute();
-  }
+  // JS15 — `getPublicJourney`'s gallery read moved to
+  // `JourneyPhotosRepository.galleryRead` (L2, above).
 }
@@ -67,7 +67,8 @@ export class TripPhotosRepository extends TrekRepository<TripPhotos> {
  /**
   * PH10's non-journey half — `SELECT 1 FROM trip_photos WHERE photo_id = ?
   * LIMIT 1`, used by `TrekPhotoRegistrationService.deleteIfOrphan` alongside
-   * the still-raw `journey_photos` existence check (`// PH10 — Plan 3g`).
+   * the `journey_photos` half of the same orphan check, converted onto
+   * {@link JourneyPhotosRepository.existsForPhoto} (Plan 3g Task 4).
   */
  async existsForPhoto(photo_id: number): Promise<boolean> {
    const row = await this.findOne({ photo: photo_id }, { fields: ['id'] });
@@ -3,6 +3,7 @@ import { InjectRepository } from '@mikro-orm/nestjs';
 import type { BookRecord, BookSummary } from '@trek/shared';
 import { normalizeBookDocument } from '@trek/shared';
 import { JourneyDomainService } from './journey-domain.service';
+import { UnitOfWork } from '../database/unit-of-work';
 import { JourneyBooks } from '../../db/entities/JourneyBooks.entity';
 import type { JourneyBookRow, JourneyBooksRepository } from '../../db/repositories/JourneyBooks.repository';

@@ -33,6 +34,9 @@ export class JourneyBookService {
  constructor(
    private readonly journey: JourneyDomainService,
    @InjectRepository(JourneyBooks) private readonly booksRepo: JourneyBooksRepository,
+    // M1 (task-5-review.md) — `saveBook`'s first-save read-then-insert
+    // needs the connection mutex to serialize two concurrent first saves.
+    private readonly uow: UnitOfWork,
  ) {}

  /** Null when the journey does not exist or the user cannot reach it. */
@@ -117,38 +121,50 @@ export class JourneyBookService {
    if (!(await this.canWrite(journeyId, userId))) return null;

    const document = JSON.stringify(normalizeBookDocument(input.document));
-    // JB3.
-    const existing = await this.booksRepo.findFirstForJourney(journeyId);

-    if (!existing) {
-      // JB4.
-      const id = await this.booksRepo.insertBook(journeyId, input.title, document, userId);
-      return { record: (await this.byId(id))! };
-    }
+    // M1 (rule 11/24) — JB3's existing-link read and JB4's insert are
+    // separate awaits, so two concurrent first saves of the same journey's
+    // book could both read "no book yet" and both insert, leaving one
+    // editor's save silently orphaned behind `getBook`'s `ORDER BY id LIMIT
+    // 1` (there is no unique index on `journey_id`). Wrapped whole: every
+    // statement below is DB-only, and the mutex serializes a second
+    // caller's read behind the first's commit, so it finds the row the
+    // first just created and takes the JB5 CAS-update path instead, the
+    // same outcome the synchronous legacy code always had.
+    return this.uow.transactional(async () => {
+      // JB3.
+      const existing = await this.booksRepo.findFirstForJourney(journeyId);

-    /*
-     * The version goes in the WHERE clause rather than being checked first.
-     * Read-then-write leaves a window between the two in which another save can
-     * land, and SQLite gives no guarantee across two statements — one UPDATE
-     * that matches on the version cannot lose that race with itself.
-     *
-     * A save with no base version is a first write from a client that has not
-     * loaded one; it is allowed to take the current version, since refusing it
-     * would break "open Studio and start editing" for the second person to
-     * arrive.
-     */
-    const base = input.baseVersion ?? existing.version;
-    // JB5 (R2) — one conditional UPDATE, returning the affected-row count.
-    const changes = await this.booksRepo.casUpdate(existing.id, base, {
-      title: input.title,
-      document,
-      updatedBy: userId,
+      if (!existing) {
+        // JB4.
+        const id = await this.booksRepo.insertBook(journeyId, input.title, document, userId);
+        return { record: (await this.byId(id))! };
+      }
+
+      /*
+       * The version goes in the WHERE clause rather than being checked first.
+       * Read-then-write leaves a window between the two in which another save can
+       * land, and SQLite gives no guarantee across two statements — one UPDATE
+       * that matches on the version cannot lose that race with itself.
+       *
+       * A save with no base version is a first write from a client that has not
+       * loaded one; it is allowed to take the current version, since refusing it
+       * would break "open Studio and start editing" for the second person to
+       * arrive.
+       */
+      const base = input.baseVersion ?? existing.version;
+      // JB5 (R2) — one conditional UPDATE, returning the affected-row count.
+      const changes = await this.booksRepo.casUpdate(existing.id, base, {
+        title: input.title,
+        document,
+        updatedBy: userId,
+      });
+
+      if (changes === 0) {
+        return { conflict: (await this.byId(existing.id))! };
+      }
+      return { record: (await this.byId(existing.id))! };
    });
-
-    if (changes === 0) {
-      return { conflict: (await this.byId(existing.id))! };
-    }
-    return { record: (await this.byId(existing.id))! };
  }

  async deleteBook(journeyId: number, userId: number): Promise<boolean | null> {
@@ -460,43 +460,51 @@ export class JourneyDomainService {
  // ── Sync engine ──────────────────────────────────────────────────────────

  async syncTripPlaces(journeyId: number, tripId: number, authorId: number) {
-    const places = await this.journeyTripsRepo.listAssignedPlacesForTrip(tripId);
+    // M2 (rule 11/24) — the existence read and the skeleton insert are
+    // separate awaits now that the sync engine is async, so two concurrent
+    // syncs for the same trip could both miss the same place and each insert
+    // a skeleton for it. Wrapped whole: every statement inside is DB-only
+    // (repository calls), and the connection mutex serializes a second
+    // caller's statements behind this one until it commits.
+    await this.uow.transactional(async () => {
+      const places = await this.journeyTripsRepo.listAssignedPlacesForTrip(tripId);

-    const now = this.ts();
-    const existing = await this.entriesRepo.listSourceKeysForTrip(journeyId, tripId);
-    const existingKeys = new Set(existing.map((e) => skeletonKey(e.source_place_id, e.source_assignment_id)));
+      const now = this.ts();
+      const existing = await this.entriesRepo.listSourceKeysForTrip(journeyId, tripId);
+      const existingKeys = new Set(existing.map((e) => skeletonKey(e.source_place_id, e.source_assignment_id)));

-    // Track next sort_order per date so synced skeletons get unique, sequential positions.
-    const dateMaxOrder = new Map<string, number>();
-    const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId);
-    for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m);
+      // Track next sort_order per date so synced skeletons get unique, sequential positions.
+      const dateMaxOrder = new Map<string, number>();
+      const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId);
+      for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m);

-    for (const place of places) {
-      const key = skeletonKey(place.id, place.assignment_id);
-      if (existingKeys.has(key)) continue;
-      existingKeys.add(key);
+      for (const place of places) {
+        const key = skeletonKey(place.id, place.assignment_id);
+        if (existingKeys.has(key)) continue;
+        existingKeys.add(key);

-      const entryDate = place.day_date || new Date().toISOString().split('T')[0];
-      const entryTime = place.assignment_time || place.place_time || null;
-      const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1;
-      dateMaxOrder.set(entryDate, nextOrder);
+        const entryDate = place.day_date || new Date().toISOString().split('T')[0];
+        const entryTime = place.assignment_time || place.place_time || null;
+        const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1;
+        dateMaxOrder.set(entryDate, nextOrder);

-      await this.insertSkeletonEntry({
-        journeyId,
-        tripId,
-        placeId: place.id,
-        assignmentId: place.assignment_id ?? null,
-        authorId,
-        title: place.name,
-        entryDate,
-        entryTime,
-        locationName: place.address || place.name,
-        lat: place.lat || null,
-        lng: place.lng || null,
-        sortOrder: nextOrder,
-        now,
-      });
-    }
+        await this.insertSkeletonEntry({
+          journeyId,
+          tripId,
+          placeId: place.id,
+          assignmentId: place.assignment_id ?? null,
+          authorId,
+          title: place.name,
+          entryDate,
+          entryTime,
+          locationName: place.address || place.name,
+          lat: place.lat || null,
+          lng: place.lng || null,
+          sortOrder: nextOrder,
+          now,
+        });
+      }
+    });
  }

  // called when a trip place is created
@@ -511,38 +519,44 @@ export class JourneyDomainService {
    if (!assignments.length) return; // not assigned to a day yet — skip

    const now = this.ts();
-    for (const journeyId of links) {
-      // JG40 — same text as JG2/JG54, resolved through `findOwnerId`. No
-      // undefined-guard here (matching the legacy statement's own unchecked
-      // `as { user_id: number }` cast): `journey_trips` cascades on the
-      // owning journey's delete, so a linked journey can never be missing.
-      const ownerId = (await this.journeysRepo.findOwnerId(journeyId)) as number;
+    // M2 (rule 11/24) — same fix as syncTripPlaces: the existence check
+    // (`existsForPlaceAssignment`) and the insert are separate awaits, so a
+    // concurrent reconcile racing this callback could both find nothing and
+    // both insert. Wrapped whole; DB-only, no broadcast inside.
+    await this.uow.transactional(async () => {
+      for (const journeyId of links) {
+        // JG40 — same text as JG2/JG54, resolved through `findOwnerId`. No
+        // undefined-guard here (matching the legacy statement's own unchecked
+        // `as { user_id: number }` cast): `journey_trips` cascades on the
+        // owning journey's delete, so a linked journey can never be missing.
+        const ownerId = (await this.journeysRepo.findOwnerId(journeyId)) as number;

-      for (const place of assignments) {
-        const already = await this.entriesRepo.existsForPlaceAssignment(journeyId, placeId, place.assignment_id ?? null);
-        if (already) continue;
+        for (const place of assignments) {
+          const already = await this.entriesRepo.existsForPlaceAssignment(journeyId, placeId, place.assignment_id ?? null);
+          if (already) continue;

-        const entryDate = place.day_date as string;
-        const maxOrder = await this.entriesRepo.maxSortOrderForDate(journeyId, entryDate);
-        const nextOrder = (maxOrder ?? -1) + 1;
+          const entryDate = place.day_date as string;
+          const maxOrder = await this.entriesRepo.maxSortOrderForDate(journeyId, entryDate);
+          const nextOrder = (maxOrder ?? -1) + 1;

-        await this.insertSkeletonEntry({
-          journeyId,
-          tripId,
-          placeId,
-          assignmentId: place.assignment_id ?? null,
-          authorId: ownerId,
-          title: place.name,
-          entryDate,
-          entryTime: place.assignment_time || place.place_time || null,
-          locationName: place.address || place.name,
-          lat: place.lat || null,
-          lng: place.lng || null,
-          sortOrder: nextOrder,
-          now,
-        });
+          await this.insertSkeletonEntry({
+            journeyId,
+            tripId,
+            placeId,
+            assignmentId: place.assignment_id ?? null,
+            authorId: ownerId,
+            title: place.name,
+            entryDate,
+            entryTime: place.assignment_time || place.place_time || null,
+            locationName: place.address || place.name,
+            lat: place.lat || null,
+            lng: place.lng || null,
+            sortOrder: nextOrder,
+            now,
+          });
+        }
      }
-    }
+    });
  }

  // called when a trip place is updated
@@ -695,137 +709,148 @@ export class JourneyDomainService {

    const now = this.ts();
    for (const journeyId of links) {
-      const ownerId = await this.journeysRepo.findOwnerId(journeyId);
-      if (ownerId === undefined) continue;
+      // M2 (rule 11/24) — the existence read (`listForTripReconcile`) and the
+      // upsert/delete writes below are separate awaits, so two concurrent
+      // reconciles for the same trip (or a reconcile racing `onPlaceCreated`)
+      // could both read "not there yet" and both insert. Wrapped whole per
+      // journey: every statement inside is DB-only, and the mutex serializes
+      // a second caller's statements behind this one until it commits — the
+      // broadcast (non-DB I/O) stays outside, after commit, per rule 24.
+      const changed = await this.uow.transactional(async () => {
+        const ownerId = await this.journeysRepo.findOwnerId(journeyId);
+        if (ownerId === undefined) return false;

-      let changed = false;
-      // JG55 — a wider column set than JG35 (`listSourceKeysForTrip`), not a dup.
-      const existing = await this.entriesRepo.listForTripReconcile(journeyId, tripId);
-      const existingByKey = new Map<string, (typeof existing)[number]>();
-      // Rows from before the assignment link existed, and any the backfill could not
-      // resolve. The place's earliest assignment claims one below, rather than the row
-      // being read as "no longer in the plan" and annotated out from under its author.
-      const unclaimed = new Map<number, (typeof existing)[number][]>();
-      for (const e of existing) {
-        if (e.source_place_id == null) continue;
-        if (e.source_assignment_id == null) {
-          const pool = unclaimed.get(e.source_place_id);
-          if (pool) pool.push(e);
-          else unclaimed.set(e.source_place_id, [e]);
-        } else {
-          existingByKey.set(skeletonKey(e.source_place_id, e.source_assignment_id), e);
-        }
-      }
-
-      // Next sort_order per date for freshly inserted skeletons.
-      const dateMaxOrder = new Map<string, number>();
-      const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId);
-      for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m);
-
-      // 1) Upsert a skeleton for every current day assignment.
-      for (const place of places) {
-        const entryDate = place.day_date || new Date().toISOString().split('T')[0];
-        const entryTime = place.assignment_time || place.place_time || null;
-        const locationName = place.address || place.name;
-        const lat = place.lat || null;
-        const lng = place.lng || null;
-        let found = existingByKey.get(skeletonKey(place.id, place.assignment_id));
-
-        if (!found) {
-          // `places` is ordered by day, so the earliest assignment claims it.
-          const adopted = unclaimed.get(place.id)?.shift();
-          if (adopted) {
-            await this.entriesRepo.claimAssignment(adopted.id, place.assignment_id ?? null);
-            adopted.source_assignment_id = place.assignment_id ?? null;
-            existingByKey.set(skeletonKey(place.id, place.assignment_id), adopted);
-            found = adopted;
+        let didChange = false;
+        // JG55 — a wider column set than JG35 (`listSourceKeysForTrip`), not a dup.
+        const existing = await this.entriesRepo.listForTripReconcile(journeyId, tripId);
+        const existingByKey = new Map<string, (typeof existing)[number]>();
+        // Rows from before the assignment link existed, and any the backfill could not
+        // resolve. The place's earliest assignment claims one below, rather than the row
+        // being read as "no longer in the plan" and annotated out from under its author.
+        const unclaimed = new Map<number, (typeof existing)[number][]>();
+        for (const e of existing) {
+          if (e.source_place_id == null) continue;
+          if (e.source_assignment_id == null) {
+            const pool = unclaimed.get(e.source_place_id);
+            if (pool) pool.push(e);
+            else unclaimed.set(e.source_place_id, [e]);
+          } else {
+            existingByKey.set(skeletonKey(e.source_place_id, e.source_assignment_id), e);
          }
        }

-        if (!found) {
-          const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1;
-          dateMaxOrder.set(entryDate, nextOrder);
-          await this.insertSkeletonEntry({
-            journeyId,
-            tripId,
-            placeId: place.id,
-            assignmentId: place.assignment_id ?? null,
-            authorId: ownerId,
-            title: place.name,
-            entryDate,
-            entryTime,
-            locationName,
-            lat,
-            lng,
-            sortOrder: nextOrder,
-            now,
-          });
-          changed = true;
-        } else if (found.type === 'skeleton') {
-          // Skeletons follow the place's day/time/location snapshot.
-          const stale =
-            found.title !== place.name ||
-            found.entry_date !== entryDate ||
-            found.entry_time !== entryTime ||
-            found.location_name !== locationName ||
-            found.location_lat !== lat ||
-            found.location_lng !== lng;
-          if (stale) {
-            await this.entriesRepo.updateSkeletonSnapshot(found.id, {
+        // Next sort_order per date for freshly inserted skeletons.
+        const dateMaxOrder = new Map<string, number>();
+        const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId);
+        for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m);
+
+        // 1) Upsert a skeleton for every current day assignment.
+        for (const place of places) {
+          const entryDate = place.day_date || new Date().toISOString().split('T')[0];
+          const entryTime = place.assignment_time || place.place_time || null;
+          const locationName = place.address || place.name;
+          const lat = place.lat || null;
+          const lng = place.lng || null;
+          let found = existingByKey.get(skeletonKey(place.id, place.assignment_id));
+
+          if (!found) {
+            // `places` is ordered by day, so the earliest assignment claims it.
+            const adopted = unclaimed.get(place.id)?.shift();
+            if (adopted) {
+              await this.entriesRepo.claimAssignment(adopted.id, place.assignment_id ?? null);
+              adopted.source_assignment_id = place.assignment_id ?? null;
+              existingByKey.set(skeletonKey(place.id, place.assignment_id), adopted);
+              found = adopted;
+            }
+          }
+
+          if (!found) {
+            const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1;
+            dateMaxOrder.set(entryDate, nextOrder);
+            await this.insertSkeletonEntry({
+              journeyId,
+              tripId,
+              placeId: place.id,
+              assignmentId: place.assignment_id ?? null,
+              authorId: ownerId,
              title: place.name,
-              entry_date: entryDate,
-              entry_time: entryTime,
-              location_name: locationName,
-              location_lat: lat,
-              location_lng: lng,
-              country_code: this.countryFor(lat, lng),
-              updated_at: now,
+              entryDate,
+              entryTime,
+              locationName,
+              lat,
+              lng,
+              sortOrder: nextOrder,
+              now,
            });
-            changed = true;
-          }
-        } else {
-          // Filled entries keep the user's date/story; only location follows the place.
-          const stale =
-            found.location_name !== locationName || found.location_lat !== lat || found.location_lng !== lng;
-          if (stale) {
-            await this.entriesRepo.updateLocationOnly(found.id, {
-              location_name: locationName,
-              location_lat: lat,
-              location_lng: lng,
-              country_code: this.countryFor(lat, lng),
-              updated_at: now,
-            });
-            changed = true;
+            didChange = true;
+          } else if (found.type === 'skeleton') {
+            // Skeletons follow the place's day/time/location snapshot.
+            const stale =
+              found.title !== place.name ||
+              found.entry_date !== entryDate ||
+              found.entry_time !== entryTime ||
+              found.location_name !== locationName ||
+              found.location_lat !== lat ||
+              found.location_lng !== lng;
+            if (stale) {
+              await this.entriesRepo.updateSkeletonSnapshot(found.id, {
+                title: place.name,
+                entry_date: entryDate,
+                entry_time: entryTime,
+                location_name: locationName,
+                location_lat: lat,
+                location_lng: lng,
+                country_code: this.countryFor(lat, lng),
+                updated_at: now,
+              });
+              didChange = true;
+            }
+          } else {
+            // Filled entries keep the user's date/story; only location follows the place.
+            const stale =
+              found.location_name !== locationName || found.location_lat !== lat || found.location_lng !== lng;
+            if (stale) {
+              await this.entriesRepo.updateLocationOnly(found.id, {
+                location_name: locationName,
+                location_lat: lat,
+                location_lng: lng,
+                country_code: this.countryFor(lat, lng),
+                updated_at: now,
+              });
+              didChange = true;
+            }
          }
        }
-      }

-      // 2) Drop skeletons whose assignment is gone. One still waiting to be claimed is
-      //    spared while its place is on the plan somewhere: having no link yet is not
-      //    the same as the stop having left.
-      for (const e of existing) {
-        if (e.source_place_id == null) continue;
-        if (e.source_assignment_id == null && assignedPlaceIds.has(e.source_place_id)) continue;
-        if (e.source_assignment_id != null && assignedKeys.has(skeletonKey(e.source_place_id, e.source_assignment_id))) {
-          continue;
-        }
-        if (e.type === 'skeleton') {
-          const hasPhotos = await this.entriesRepo.existsPhotoForEntry(e.id);
-          if (!hasPhotos && !e.story) {
-            await this.entriesRepo.deleteById(e.id);
-            changed = true;
+        // 2) Drop skeletons whose assignment is gone. One still waiting to be claimed is
+        //    spared while its place is on the plan somewhere: having no link yet is not
+        //    the same as the stop having left.
+        for (const e of existing) {
+          if (e.source_place_id == null) continue;
+          if (e.source_assignment_id == null && assignedPlaceIds.has(e.source_place_id)) continue;
+          if (e.source_assignment_id != null && assignedKeys.has(skeletonKey(e.source_place_id, e.source_assignment_id))) {
            continue;
          }
+          if (e.type === 'skeleton') {
+            const hasPhotos = await this.entriesRepo.existsPhotoForEntry(e.id);
+            if (!hasPhotos && !e.story) {
+              await this.entriesRepo.deleteById(e.id);
+              didChange = true;
+              continue;
+            }
+          }
+          const note = '\n\n> _Note: the original trip place was removed from the trip plan_';
+          const newStory = (e.story || '') + note;
+          await this.entriesRepo.detachAndAnnotate(e.id, {
+            type: e.type === 'skeleton' ? 'entry' : e.type,
+            story: newStory,
+            updated_at: now,
+          });
+          didChange = true;
        }
-        const note = '\n\n> _Note: the original trip place was removed from the trip plan_';
-        const newStory = (e.story || '') + note;
-        await this.entriesRepo.detachAndAnnotate(e.id, {
-          type: e.type === 'skeleton' ? 'entry' : e.type,
-          story: newStory,
-          updated_at: now,
-        });
-        changed = true;
-      }
+
+        return didChange;
+      });

      if (changed) await this.broadcastJourneyEvent(journeyId, 'journey:trip:synced', { tripId }, sid);
    }
@@ -1273,13 +1298,16 @@ export class JourneyDomainService {
      updated_at: [true, this.ts()],
    });

-    // The legacy no-op check (`fields.length === 0`) fires exactly when NO
-    // allow-listed key of `data` carries a defined value — the country
-    // recompute and the type-promotion push are both themselves gated on
-    // keys already in that same set (`location_lat`/`location_lng`/`story`),
-    // so "any key of `data` is defined" is the identical condition.
-    const anyFieldProvided = Object.values(data).some((v) => v !== undefined);
-    if (!anyFieldProvided) return decodeEntryRow(entry);
+    // M3 (parity) — the legacy no-op check (`fields.length === 0`) fires
+    // exactly when NO allow-listed key made it into the dynamic SET list.
+    // `Object.values(data).some(v => v !== undefined)` is the WRONG mirror
+    // of that: `data` is `journeyEntryUpdateRequestSchema`'s `z.looseObject`,
+    // so a PATCH body carrying only non-allow-listed keys (`{"foo":1}`) has
+    // a defined value there and this used to write and broadcast anyway.
+    // `patch` (built above by `presenceSet`) always carries `updated_at`
+    // unconditionally, so its length is 1 exactly when nothing allow-listed
+    // was provided — the same condition the legacy field-count check tested.
+    if (Object.keys(patch).length === 1) return decodeEntryRow(entry);

    await this.entriesRepo.updateFields(entryId, patch);

@@ -4,6 +4,7 @@ import crypto from 'crypto';
 import { JourneyDomainService } from './journey-domain.service';
 import { decodeEntryRow } from './journey-entry-row';
 import { SettingsService } from '../settings/settings.service';
+import { UnitOfWork } from '../database/unit-of-work';
 import { Journeys } from '../../db/entities/Journeys.entity';
 import type { JourneysRepository } from '../../db/repositories/Journeys.repository';
 import { JourneyShareTokens } from '../../db/entities/JourneyShareTokens.entity';
@@ -11,6 +12,8 @@ import type {
  JourneyPublicEntryPhotoRow,
  JourneyShareTokensRepository,
 } from '../../db/repositories/JourneyShareTokens.repository';
+import { JourneyPhotos } from '../../db/entities/JourneyPhotos.entity';
+import type { JourneyPhotosRepository } from '../../db/repositories/JourneyPhotos.repository';

 interface JourneySharePermissions {
  share_timeline?: boolean;
@@ -47,6 +50,17 @@ export class JourneyShareService {
    // already-stable `JourneysRepository.findById` (the JG5 dup group) rather
    // than a second, hand-kept copy of the same statement.
    @InjectRepository(Journeys) private readonly journeysRepo: JourneysRepository,
+    // Fix-wave additions (task-5-review.md):
+    // - `uow` (L1): `createOrUpdateJourneyShareLink`'s existing-link read and
+    //   its insert are separate awaits now, so two concurrent first-creates
+    //   for the same journey could both read "no link yet" and both insert —
+    //   the second hits `UNIQUE(journey_id)` and 500s instead of returning
+    //   `{created:false}` like the legacy synchronous path always did.
+    // - `photosRepo` (L2): `getPublicJourney`'s gallery read (JS15) reuses
+    //   `JourneyPhotosRepository.galleryRead` instead of carrying its own
+    //   byte-identical copy of the ORDER BY builder and the gallery query.
+    private readonly uow: UnitOfWork,
+    @InjectRepository(JourneyPhotos) private readonly photosRepo: JourneyPhotosRepository,
  ) {}

  async createOrUpdateJourneyShareLink(
@@ -58,45 +72,55 @@ export class JourneyShareService {
    // able to publish the journey or change which screens are shared.
    if (!(await this.journey.isOwner(journeyId, createdBy))) return null;

-    // JS1 — `JourneyShareTokensRepository.findFlagsByJourneyId`.
-    const existing = await this.shareTokensRepo.findFlagsByJourneyId(journeyId);
+    // L1 (rule 11/24) — the existing-link read and the insert below are
+    // separate awaits, so two concurrent first-creates for the same journey
+    // could both read "no link yet" and both try to insert: the loser hits
+    // `UNIQUE(journey_id)` and throws (a 500 over HTTP) instead of the
+    // legacy's `{created:false}` outcome. Wrapped whole, DB-only, same fix
+    // as M1: the mutex serializes the second caller's read behind the
+    // first's commit, so it finds the row the first just created and takes
+    // the UPDATE branch instead, exactly like the synchronous legacy path.
+    return this.uow.transactional(async () => {
+      // JS1 — `JourneyShareTokensRepository.findFlagsByJourneyId`.
+      const existing = await this.shareTokensRepo.findFlagsByJourneyId(journeyId);

-    if (existing) {
-      // An update only changes the flags it was actually given. Falling back to
-      // the create-time defaults here would silently re-publish a gallery or map
-      // the owner had switched off, at the unchanged token.
-      const share_timeline = permissions.share_timeline ?? !!existing.share_timeline;
-      const share_gallery = permissions.share_gallery ?? !!existing.share_gallery;
-      const share_map = permissions.share_map ?? !!existing.share_map;
-      const newest_first = permissions.newest_first ?? !!existing.newest_first;
-      // JS2 — the SERVICE resolves the final value of every flag (above); the
-      // repository writes exactly those four already-resolved booleans, a
-      // plain 4-column UPDATE, not a presence-sentinel one.
-      await this.shareTokensRepo.updateFlags(journeyId, {
+      if (existing) {
+        // An update only changes the flags it was actually given. Falling back to
+        // the create-time defaults here would silently re-publish a gallery or map
+        // the owner had switched off, at the unchanged token.
+        const share_timeline = permissions.share_timeline ?? !!existing.share_timeline;
+        const share_gallery = permissions.share_gallery ?? !!existing.share_gallery;
+        const share_map = permissions.share_map ?? !!existing.share_map;
+        const newest_first = permissions.newest_first ?? !!existing.newest_first;
+        // JS2 — the SERVICE resolves the final value of every flag (above); the
+        // repository writes exactly those four already-resolved booleans, a
+        // plain 4-column UPDATE, not a presence-sentinel one.
+        await this.shareTokensRepo.updateFlags(journeyId, {
+          share_timeline: share_timeline ? 1 : 0,
+          share_gallery: share_gallery ? 1 : 0,
+          share_map: share_map ? 1 : 0,
+          newest_first: newest_first ? 1 : 0,
+        });
+        return { token: existing.token, created: false };
+      }
+
+      const {
+        share_timeline = true,
+        share_gallery = true,
+        share_map = true,
+        newest_first = false,
+      } = permissions;
+
+      const token = crypto.randomBytes(24).toString('base64url');
+      // JS3.
+      await this.shareTokensRepo.insertLink(journeyId, token, createdBy, {
        share_timeline: share_timeline ? 1 : 0,
        share_gallery: share_gallery ? 1 : 0,
        share_map: share_map ? 1 : 0,
        newest_first: newest_first ? 1 : 0,
      });
-      return { token: existing.token, created: false };
-    }
-
-    const {
-      share_timeline = true,
-      share_gallery = true,
-      share_map = true,
-      newest_first = false,
-    } = permissions;
-
-    const token = crypto.randomBytes(24).toString('base64url');
-    // JS3.
-    await this.shareTokensRepo.insertLink(journeyId, token, createdBy, {
-      share_timeline: share_timeline ? 1 : 0,
-      share_gallery: share_gallery ? 1 : 0,
-      share_map: share_map ? 1 : 0,
-      newest_first: newest_first ? 1 : 0,
+      return { token, created: true };
    });
-    return { token, created: true };
  }

  /**
@@ -194,8 +218,10 @@ export class JourneyShareService {
      (photosByEntry[p.entry_id] ||= []).push(p);
    }

-    // JS15 (R1's second `GALLERY_CHRONOLOGICAL_ORDER` site).
-    const gallery = await this.shareTokensRepo.listGalleryForPublicJourney(row.journey_id);
+    // JS15 (R1's second `GALLERY_CHRONOLOGICAL_ORDER` site) — L2: reuses
+    // `JourneyPhotosRepository.galleryRead` (same `GALLERY_COLUMNS`, same
+    // ORDER BY) rather than a second, hand-kept copy of the same query.
+    const gallery = await this.photosRepo.galleryRead(row.journey_id);

    const enrichedEntries = entries
      .map(e => ({
@@ -125,6 +125,7 @@ import {
  createTestDaysRepo, createTestDayAssignmentsRepo, createTestDayNotesRepo, createTestTripsRepo,
  createTestTripMembersRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo,
  createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, createTestRoadtripViasRepo, createTestRoadtripDayTracksRepo,
+  createTestPlaceDetailsCacheRepo,
  createTestReservationsRepo,
  createTestReservationEndpointsRepo,
  createTestReservationTravelersRepo,
@@ -251,7 +252,7 @@ export async function createMcpTestRegistry(): Promise<McpRegistry> {
  // Exactly one instance, shared by maps, places and share: its stampede guard
  // and its on-disk set only work if all three readers see the same maps.
  const placePhotoCache = new PlacePhotoCacheService(dbService, makeStorageFixture('photos/google/').storage, await createTestGooglePlacePhotoMetaRepo(dbService.connection), await createTestPlacesRepo(dbService.connection));
-  const mapsService = new MapsService(dbService, placePhotoCache, appSettings, usersRepo);
+  const mapsService = new MapsService(placePhotoCache, appSettings, usersRepo, await createTestPlaceDetailsCacheRepo(dbService.connection), await createTestPlacesRepo(dbService.connection));
  const journeyDomain = new JourneyDomainService(
    dbService, realtimeService, new TrekPhotoRegistrationService(mcpOrm.repo(TrekPhotos), mcpOrm.repo(TripPhotos), await createTestJourneyPhotosRepo(dbService.connection), dbService), await createTestUnitOfWork(dbService.connection),
    // Plan 3g Task 1 — the constructor-ripple fix (R9): four journey-owned
@@ -472,11 +473,16 @@ export async function createMcpTestRegistry(): Promise<McpRegistry> {
        // Plan 3g Task 3 — the constructor-ripple fix: `JourneyShareTokensRepository`
        // (JS1-JS15) + the already-shared `JourneysRepository` (JS8/JS12).
        await createTestJourneyShareTokensRepo(dbService.connection), await createTestJourneysRepo(dbService.connection),
+        // task-5-fix-brief constructor-ripple: `UnitOfWork` (L1) + `JourneyPhotosRepository` (L2).
+        await createTestUnitOfWork(dbService.connection), await createTestJourneyPhotosRepo(dbService.connection),
      ), addonsService, authService, captureBackfill),
      new MemoriesMcp(immichService, synologyService, addonsService, mcpOrm.repo(PhotoProviders)),
      new NotificationsMcp(await makeNotificationsService(dbService, realtimeService), authService),
-      new AirtrailMcp(new AirtrailService(dbService, new AuditService(auditLogRepo, usersRepo), new AirtrailClient()), addonsService),
-      new ReservationImportMcp(new AirtrailImportService(dbService, realtimeService, reservationsService, new AirtrailClient(), new AirtrailService(dbService, new AuditService(auditLogRepo, usersRepo), new AirtrailClient())), dbService, authService, guards, addonsService),
+      new AirtrailMcp(new AirtrailService(usersRepo, new AuditService(auditLogRepo, usersRepo), new AirtrailClient()), addonsService),
+      new ReservationImportMcp(new AirtrailImportService(
+        await createTestReservationsRepo(dbService.connection), await createTestReservationEndpointsRepo(dbService.connection), await createTestDaysRepo(dbService.connection),
+        realtimeService, reservationsService, new AirtrailClient(), new AirtrailService(usersRepo, new AuditService(auditLogRepo, usersRepo), new AirtrailClient()),
+      ), dbService, authService, guards, addonsService),
      new SettingsMcp(new SettingsService(await createTestUnitOfWork(dbService.connection), appSettings, await createTestSettingsRepo(dbService.connection)), authService),
      new HelpMcp(), new AddonsMcp(addonsService),
      new TripWarningsMcp(new PluginHooks({ providersOf: () => [], invokeHook: async () => [] } as unknown as PluginRuntimeService), dbService),
@@ -81,6 +81,7 @@ import {
  createTestDaysRepo, createTestDayAssignmentsRepo, createTestDayNotesRepo, createTestTripsRepo,
  createTestTripMembersRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo,
  createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, createTestRoadtripViasRepo,
+  createTestPlaceDetailsCacheRepo,
  createTestReservationsRepo,
  createTestReservationEndpointsRepo,
  createTestReservationTravelersRepo,
@@ -253,7 +254,7 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise<
  );
  // After it: deleting a place cancels the nights booked at it through this one.
  const places = new PlacesService(
-    dbs, permissions, realtime, new MapsService(dbs, photoCache, appSettings, usersRepo), queryHelpers, unsplash, photoCache, journey, generalStorage, accommodations, await createTestUnitOfWork(dbs.connection),
+    dbs, permissions, realtime, new MapsService(photoCache, appSettings, usersRepo, await createTestPlaceDetailsCacheRepo(dbs.connection), await createTestPlacesRepo(dbs.connection)), queryHelpers, unsplash, photoCache, journey, generalStorage, accommodations, await createTestUnitOfWork(dbs.connection),
    await createTestPlacesRepo(dbs.connection),
    await createTestTagsRepo(dbs.connection),
    await createTestPlaceRatingsRepo(dbs.connection),
@@ -81,7 +81,9 @@ beforeAll(async () => {
    await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb),
  );
  // Plan 3g Task 3: JourneyBooksRepository (JB1-JB7), not `dbs` any more.
-  books = new JourneyBookService(domain, await createTestJourneyBooksRepo(testDb));
+  // task-5-fix-brief constructor-ripple (M1): `UnitOfWork`, so `saveBook`'s
+  // read-then-insert can be wrapped in one transaction.
+  books = new JourneyBookService(domain, await createTestJourneyBooksRepo(testDb), uow);
 });

 beforeEach(() => {
@@ -351,6 +353,37 @@ describe('concurrency', () => {
    const afterMutation = testDb.prepare('SELECT title FROM journey_books WHERE id = ?').get(row.id) as { title: string };
    expect(afterMutation.title).toBe('Clobbered');
  });
+
+  // M1 (task-5-review.md) — JB3's existing-link read and JB4's insert used
+  // to be two separate awaits, so two concurrent FIRST saves of a journey
+  // with no book yet could both read "no book" and both insert, leaving one
+  // editor's save orphaned behind `getBook`'s `ORDER BY id LIMIT 1` (there is
+  // no unique index on `journey_id`). Races two REAL `await Promise.all([...])`
+  // callers through the actual service on the shared connection, then
+  // asserts the base's outcome: exactly one row, and the loser lands on the
+  // JB5 CAS-update path (version 2), not a second insert.
+  it('journey-book-svc M1: two concurrent first saves of a journey with no book — exactly one row, ending at version 2', async () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+
+    const [a, b] = await Promise.all([
+      books.saveBook(journey.id, user.id, { title: 'A', document: doc('a') }),
+      books.saveBook(journey.id, user.id, { title: 'B', document: doc('b') }),
+    ]);
+
+    expect(a).not.toBeNull();
+    expect(b).not.toBeNull();
+    // One of the two lands as the first insert (version 1, before the
+    // second's own read observes it and takes the CAS-update branch); the
+    // other either also lands as {record} (version 2, having taken the
+    // update branch against baseVersion undefined -> existing.version) or
+    // sees a stale conflict — either way, never a second inserted row.
+    const rows = testDb.prepare('SELECT COUNT(*) AS n FROM journey_books WHERE journey_id = ?').get(journey.id) as { n: number };
+    expect(rows.n).toBe(1);
+
+    const finalVersion = (testDb.prepare('SELECT version FROM journey_books WHERE journey_id = ?').get(journey.id) as { version: number }).version;
+    expect(finalVersion).toBe(2);
+  });
 });

 // -- Broadcasting -------------------------------------------------------------
@@ -60,6 +60,10 @@ import {
  createTestJourneysRepo, createTestJourneyContributorsRepo, createTestJourneyTripsRepo, createTestJourneyEntriesRepo,
  createTestJourneyPhotosRepo, createTestJourneyEntryPhotosRepo,
 } from '../../helpers/journey-repos';
+// M5d (task-5-review.md) — full-key parity tests for Task 2's read models,
+// ported from the reviewer's probe (scratchpad r3g/probe/zz-r3g-parity-probe.test.ts).
+import { todayUtc } from '@trek/shared';
+import { GALLERY_CHRONOLOGICAL_ORDER } from '../../../src/nest/journey/journey-gallery-order';

 let dbs: DatabaseService;
 let svc: JourneyDomainService;
@@ -701,6 +705,36 @@ describe('updateEntry', () => {
    expect((await svc.updateEntry(entry!.id, user.id, { title: 'Renamed' }))!.pros_cons).toEqual(prosCons);
  });

+  // L3 (task-5-review.md, user decision: KEEP HEAD's behaviour, deliberate
+  // deviation FOR THE USER) — base stored the raw non-array `tags`/non-object
+  // `pros_cons` value as-is and then 500ed on every later read that tried to
+  // JSON.parse it, leaving the entry permanently unreadable; base also 500s
+  // outright on a `false` bind. HEAD writes SQL NULL instead and answers 200,
+  // which is the narrowing that fixes that legacy defect. Pinned here so a
+  // future change to this branch is a deliberate decision, not an accident.
+  it('L3: a non-array tags / non-object pros_cons patch writes NULL and returns 200, rather than the raw value HEAD used to store (a legacy defect, not reproduced)', async () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const entry = createJourneyEntry(testDb, journey.id, user.id, { entry_date: '2026-03-01' });
+
+    const updated = await svc.updateEntry(entry.id, user.id, {
+      tags: 'not-an-array' as unknown as string[],
+      pros_cons: false as unknown as { pros: string[]; cons: string[] },
+    });
+
+    expect(updated).not.toBeNull();
+    expect(updated!.tags).toEqual([]);
+    expect(updated!.pros_cons).toBeNull();
+
+    const row = testDb.prepare('SELECT tags, pros_cons FROM journey_entries WHERE id = ?').get(entry.id) as { tags: string | null; pros_cons: string | null };
+    expect(row.tags).toBeNull();
+    expect(row.pros_cons).toBeNull();
+
+    // And the entry stays readable afterwards — no 500 on the next read,
+    // which is exactly the defect this deviation fixes.
+    expect(await svc.listEntries(journey.id, user.id)).not.toBeNull();
+  });
+
  /* An update that changes nothing takes its own early return out of the method. */
  it('decodes on the no-op update too', async () => {
    const { user } = createUser(testDb);
@@ -808,6 +842,37 @@ describe('updateEntry', () => {
    expect(updated!.story).toBe('original'); // injection key dropped — no hash leaked into story
    expect(updated!.author_id).toBe(user.id); // mass-assignment blocked
  });
+
+  // M3 (task-5-review.md) — a patch carrying ONLY non-allow-listed keys (no
+  // legit field at all, unlike JOURNEY-SVC-034b above which always mixes in
+  // a real `title`) used to be treated as "something was provided" because
+  // the old no-op check read `Object.values(data).some(v => v !== undefined)`
+  // off the RAW request body, not off the allow-listed `patch` presenceSet
+  // actually builds. Base: no write, no `updated_at` bump on either row, no
+  // broadcast. HEAD (before this fix): stamped both `updated_at`s and
+  // broadcast anyway.
+  it('JOURNEY-SVC-M3-01: a patch with only non-allow-listed keys is a no-op — no write, no updated_at bump, no broadcast', async () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const entry = createJourneyEntry(testDb, journey.id, user.id, { title: 'Original', entry_date: '2026-03-01' });
+
+    const updateFieldsSpy = vi.spyOn(entriesRepoDirect, 'updateFields');
+    const journeyUpdateSpy = vi.spyOn(journeysRepoDirect, 'updateFields');
+    const broadcastSpy = vi.spyOn(RealtimeService.prototype, 'broadcastToUser').mockImplementation(() => {});
+    try {
+      const updated = await svc.updateEntry(entry.id, user.id, { foo: 1 } as unknown as Parameters<typeof svc.updateEntry>[2]);
+
+      expect(updated).not.toBeNull();
+      expect(updated!.title).toBe('Original');
+      expect(updateFieldsSpy).not.toHaveBeenCalled();
+      expect(journeyUpdateSpy).not.toHaveBeenCalled();
+      expect(broadcastSpy).not.toHaveBeenCalled();
+    } finally {
+      updateFieldsSpy.mockRestore();
+      journeyUpdateSpy.mockRestore();
+      broadcastSpy.mockRestore();
+    }
+  });
 });

 describe('deleteEntry', () => {
@@ -1029,6 +1094,27 @@ describe('addContributor / updateContributorRole / removeContributor', () => {
    expect(result).toBe(false);
  });

+  // JG118 (task-5-review.md, user decision: pin, do NOT fix) — a PRE-EXISTING
+  // hole preserved on purpose (also present at legacy base): nothing here or
+  // in SQL stops the owner from writing `role = 'owner'` through this path —
+  // only the TypeScript parameter type discourages it, and that boundary is
+  // bypassable from a raw HTTP/MCP body. The hardening line (refuse unless
+  // `role === 'editor' || role === 'viewer'`) goes in the report FOR THE
+  // USER; this test pins today's behaviour so a future fix is a deliberate
+  // change, not a silent one.
+  it('JOURNEY-SVC-JG118-PIN: updateContributorRole still accepts role "owner" through the typed path (pre-existing, not fixed by this wave)', async () => {
+    const { user: owner } = createUser(testDb);
+    const { user: target } = createUser(testDb);
+    const journey = createJourney(testDb, owner.id);
+    addJourneyContributor(testDb, journey.id, target.id, 'editor');
+
+    const result = await svc.updateContributorRole(journey.id, owner.id, target.id, 'owner' as unknown as 'editor' | 'viewer');
+
+    expect(result).toBe(true);
+    const row = testDb.prepare('SELECT role FROM journey_contributors WHERE journey_id = ? AND user_id = ?').get(journey.id, target.id) as { role: string };
+    expect(row.role).toBe('owner');
+  });
+
  it('JOURNEY-SVC-050: owner can remove contributor', async () => {
    const { user: owner } = createUser(testDb);
    const { user: contrib } = createUser(testDb);
@@ -1059,6 +1145,109 @@ describe('addContributor / updateContributorRole / removeContributor', () => {
  });
 });

+// M5c (task-5-review.md) — R3's reset-column trap on `journey_contributors`'s
+// composite PK: the legacy `INSERT OR REPLACE` deletes and re-inserts on a
+// conflict, so `hide_skeletons` (not in the column list `upsertContributor`
+// writes on purpose) resets to its table default (0) — verified live by W2's
+// probe, but until now pinned only as report text (the ledger's "upsert SQL
+// pinned = the reference" claim did not hold). Mutation M10 (drop the
+// explicit `hide_skeletons: 0`) stayed green with no test to catch it.
+describe('R3 composite-PK writes — reset-on-conflict behaviour and rendered SQL (M5c)', () => {
+  it('JOURNEY-SVC-M5C-01: re-adding a contributor (upsertContributor merge branch) resets hide_skeletons to 0', async () => {
+    const { user: owner } = createUser(testDb);
+    const { user: contrib } = createUser(testDb);
+    const journey = createJourney(testDb, owner.id);
+
+    await svc.addContributor(journey.id, owner.id, contrib.id, 'editor');
+    await svc.updateJourneyPreferences(journey.id, contrib.id, { hide_skeletons: true });
+    expect(
+      (testDb.prepare('SELECT hide_skeletons FROM journey_contributors WHERE journey_id = ? AND user_id = ?').get(journey.id, contrib.id) as { hide_skeletons: number }).hide_skeletons,
+    ).toBe(1);
+
+    // Re-add (same target, a possibly different role) — the ON CONFLICT
+    // merge branch of upsertContributor, exercised through the real service.
+    await svc.addContributor(journey.id, owner.id, contrib.id, 'viewer');
+
+    const row = testDb
+      .prepare('SELECT role, hide_skeletons FROM journey_contributors WHERE journey_id = ? AND user_id = ?')
+      .get(journey.id, contrib.id) as { role: string; hide_skeletons: number };
+    expect(row.role).toBe('viewer');
+    expect(row.hide_skeletons).toBe(0);
+  });
+
+  it('JOURNEY-SVC-M5C-02: upsertContributor (merge-shaped) renders an ON CONFLICT over the (journey_id, user_id) composite PK', async () => {
+    const { user: owner } = createUser(testDb);
+    const { user: contrib } = createUser(testDb);
+    const journey = createJourney(testDb, owner.id);
+
+    const connection = contributorsRepoDirect.getEntityManager().getConnection();
+    const spy = vi.spyOn(connection, 'execute');
+    try {
+      await contributorsRepoDirect.upsertContributor(journey.id, contrib.id, 'editor', Date.now());
+      const insertSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql));
+      expect(insertSql).toBeDefined();
+      expect(insertSql).toMatch(/on conflict \(`journey_id`, ?`user_id`\)/i);
+      expect(insertSql).toMatch(/do update set/i);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
+  it('JOURNEY-SVC-M5C-03: JourneyTripsRepository.insertIgnore (ignore-shaped) renders an ON CONFLICT ... DO NOTHING over (journey_id, trip_id)', async () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const trip = createTrip(testDb, user.id);
+
+    const connection = journeyTripsRepoDirect.getEntityManager().getConnection();
+    const spy = vi.spyOn(connection, 'execute');
+    try {
+      await journeyTripsRepoDirect.insertIgnore(journey.id, trip.id, Date.now());
+      const insertSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql));
+      expect(insertSql).toBeDefined();
+      expect(insertSql).toMatch(/on conflict \(`journey_id`, ?`trip_id`\)/i);
+      expect(insertSql).toMatch(/do nothing/i);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
+  it('JOURNEY-SVC-M5C-04: JourneyEntryPhotosRepository.insertIgnore (ignore-shaped) renders an ON CONFLICT ... DO NOTHING over (entry_id, journey_photo_id)', async () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const entry = createJourneyEntry(testDb, journey.id, user.id);
+    const trekResult = testDb
+      .prepare('INSERT INTO trek_photos (provider, file_path, owner_id, created_at) VALUES (?, ?, ?, ?)')
+      .run('local', '/photos/m5c.jpg', user.id, Date.now());
+    const trekId = trekResult.lastInsertRowid as number;
+    const jpResult = testDb
+      .prepare('INSERT INTO journey_photos (journey_id, photo_id, sort_order, created_at) VALUES (?, ?, ?, ?)')
+      .run(journey.id, trekId, 0, Date.now());
+    const journeyPhotoId = jpResult.lastInsertRowid as number;
+
+    const connection = entryPhotosRepoDirect.getEntityManager().getConnection();
+    const spy = vi.spyOn(connection, 'execute');
+    try {
+      await entryPhotosRepoDirect.insertIgnore(entry.id, journeyPhotoId, 0, Date.now());
+      const insertSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql));
+      expect(insertSql).toBeDefined();
+      expect(insertSql).toMatch(/on conflict \(`entry_id`, ?`journey_photo_id`\)/i);
+      expect(insertSql).toMatch(/do nothing/i);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+});
+
+// M4 (task-5-review.md) — `JourneysRepository.listRecipientUserIds`'s
+// `if (ownerId !== undefined) ids.add(ownerId)` null arm had no test where
+// `findOwnerId` actually returns undefined (a missing journey), so the
+// branch went uncovered.
+describe('JourneysRepository.listRecipientUserIds — missing journey (M4)', () => {
+  it('returns an empty array for a journey id that does not exist (findOwnerId undefined arm)', async () => {
+    expect(await journeysRepoDirect.listRecipientUserIds(999_999)).toEqual([]);
+  });
+});
+
 // -- Suggestions --------------------------------------------------------------

 describe('getSuggestions', () => {
@@ -1935,6 +2124,48 @@ describe('reconcileTripSkeletons', () => {
    const anyEntry = testDb.prepare('SELECT COUNT(*) AS n FROM journey_entries').get() as { n: number };
    expect(anyEntry.n).toBe(0);
  });
+
+  // -- M2 (task-5-review.md) — the sync engine's existence-check-then-insert
+  // used to be two separate awaits, so two concurrent callers could both see
+  // "not there yet" and both insert a skeleton for the same (place,
+  // assignment). Races two REAL `await Promise.all([...])` callers through
+  // the actual service on the shared connection, then asserts the base's
+  // outcome: exactly one skeleton per (place, assignment), never two.
+  describe('concurrency (M2) — exactly one skeleton per (place, assignment) under a race', () => {
+    it('two concurrent reconcileTripSkeletons(trip) calls after two new assignments produce exactly one skeleton per place, not two', async () => {
+      const { journey, trip } = await linkedJourneyTrip();
+      const days = daysOf(trip.id);
+      const placeA = createPlace(testDb, trip.id, { name: 'Race Place A' });
+      const placeB = createPlace(testDb, trip.id, { name: 'Race Place B' });
+      createDayAssignment(testDb, days[0].id, placeA.id);
+      createDayAssignment(testDb, days[1].id, placeB.id);
+
+      await Promise.all([
+        svc.reconcileTripSkeletons(trip.id),
+        svc.reconcileTripSkeletons(trip.id),
+      ]);
+
+      const countFor = (placeId: number) =>
+        (testDb.prepare('SELECT COUNT(*) AS n FROM journey_entries WHERE journey_id = ? AND source_place_id = ?').get(journey.id, placeId) as { n: number }).n;
+      expect(countFor(placeA.id)).toBe(1);
+      expect(countFor(placeB.id)).toBe(1);
+    });
+
+    it('onPlaceCreated racing reconcileTripSkeletons for the same new assignment produces exactly one skeleton, not two', async () => {
+      const { journey, trip } = await linkedJourneyTrip();
+      const days = daysOf(trip.id);
+      const place = createPlace(testDb, trip.id, { name: 'Race Place C' });
+      createDayAssignment(testDb, days[0].id, place.id);
+
+      await Promise.all([
+        svc.onPlaceCreated(trip.id, place.id),
+        svc.reconcileTripSkeletons(trip.id),
+      ]);
+
+      const count = (testDb.prepare('SELECT COUNT(*) AS n FROM journey_entries WHERE journey_id = ? AND source_place_id = ?').get(journey.id, place.id) as { n: number }).n;
+      expect(count).toBe(1);
+    });
+  });
 });

 // -- the same place on two days (#2329) ---------------------------------------
@@ -2955,6 +3186,188 @@ describe('Plan 3g Task 1 — repository parity (full-key toEqual against the leg
  });
 });

+// M5d (task-5-review.md) — Task 2's read models had no full-key parity
+// tests: mutations dropping `lng` from JG15 (M19) and from JG19's gallery
+// read (M20) stayed green. Ported from the reviewer's own probe
+// (`scratchpad/r3g/probe/zz-r3g-parity-probe.test.ts`, which the review
+// names as the template), turned from console.log(OK/DIFF) reports into
+// real `toEqual` assertions against the legacy statement run raw on the
+// SAME seeded rows. One journey seeded with every tier this plan's read
+// models touch: dated + undated trips, a multi-day place, an unattached
+// gallery photo, ties on `sort_order`/`taken_at`, a video (excluded from
+// the "photograph per stop" read), a dismissed entry, and a second,
+// unrelated journey's photo (must never leak in).
+describe('Plan 3g Task 2 — repository parity (full-key toEqual against the legacy statement, M5d)', () => {
+  async function seedParityJourney() {
+    const { user: owner } = createUser(testDb);
+    const { user: member } = createUser(testDb);
+    const d = (n: number) => new Date(Date.now() - n * 86400000).toISOString().slice(0, 10);
+    const t1 = createTrip(testDb, owner.id, { title: 'T1', start_date: '2026-01-01', end_date: '2026-01-03' });
+    const t2 = createTrip(testDb, owner.id, { title: 'T2 undated' });
+    const t3 = createTrip(testDb, member.id, { title: 'T3 recent', start_date: d(10), end_date: d(5) });
+    const t4 = createTrip(testDb, owner.id, { title: 'T4 recent', start_date: d(12), end_date: d(2) });
+    testDb.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(t3.id, owner.id);
+    const days = testDb.prepare('SELECT id, date FROM days WHERE trip_id = ? ORDER BY date').all(t1.id) as { id: number }[];
+    const pA = createPlace(testDb, t1.id, { name: 'A' });
+    const pB = createPlace(testDb, t1.id, { name: 'B', lat: 1, lng: 2 });
+    createPlace(testDb, t1.id, { name: 'C unassigned' });
+    const pD = createPlace(testDb, t2.id, { name: 'D' });
+    testDb.prepare("UPDATE places SET route_geometry = '[[1,2],[3,4]]', route_color = '#f00' WHERE id IN (?, ?)").run(pA.id, pD.id);
+    createDayAssignment(testDb, days[1].id, pA.id);
+    createDayAssignment(testDb, days[0].id, pA.id);
+    createDayAssignment(testDb, days[0].id, pB.id);
+    createDayAssignment(testDb, days[2].id, pB.id, { order_index: 0 });
+    const place4 = createPlace(testDb, t4.id, { name: 'X' });
+    const d4 = createDay(testDb, t4.id, { date: d(3) });
+    createDayAssignment(testDb, d4.id, place4.id);
+    const journey = createJourney(testDb, owner.id);
+    for (const tid of [t1.id, t2.id]) {
+      testDb.prepare('INSERT INTO journey_trips (journey_id, trip_id, added_at) VALUES (?,?,1)').run(journey.id, tid);
+      await svc.syncTripPlaces(journey.id, tid, owner.id);
+    }
+    const now = Date.now();
+    const ins = testDb.prepare(
+      `INSERT INTO journey_entries (journey_id, author_id, type, title, story, entry_date, entry_time, location_name, location_lat, location_lng, sort_order, created_at, updated_at, stats_excluded, dismissed) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`,
+    );
+    const e1 = Number(ins.run(journey.id, owner.id, 'entry', 'E1', 's', '2026-01-02', '10:00', 'Paris', 48.8, 2.3, 0, now, now, 0, 0).lastInsertRowid);
+    const e2 = Number(ins.run(journey.id, owner.id, 'entry', 'E2', null, '2026-01-02', '', null, null, null, 0, now, now, 1, 0).lastInsertRowid);
+    const e3 = Number(ins.run(journey.id, owner.id, 'entry', 'E3 dismissed', null, '2026-01-01', null, 'X', null, null, 1, now, now, 0, 1).lastInsertRowid);
+    const e4 = Number(ins.run(journey.id, owner.id, 'entry', 'E4', null, '2025-12-31', null, 'Paris', 1, 1, 5, now, now, 0, 0).lastInsertRowid);
+    const tp = testDb.prepare(
+      `INSERT INTO trek_photos (provider, asset_id, owner_id, file_path, thumbnail_path, width, height, media_type, taken_at, lat, lng) VALUES (?,?,?,?,?,?,?,?,?,?,?)`,
+    );
+    const gp = testDb.prepare(`INSERT INTO journey_photos (journey_id, photo_id, caption, shared, sort_order, provider, asset_id, owner_id, created_at) VALUES (?,?,?,?,?,?,?,?,?)`);
+    const jep = testDb.prepare(`INSERT INTO journey_entry_photos (entry_id, journey_photo_id, sort_order, created_at) VALUES (?,?,?,?)`);
+    // taken_at/media_type combinations: null, empty string, two ties at the
+    // same taken_at (one video — excluded from listFirstPhotoPerEntry), and
+    // a null media_type (treated as 'image').
+    const specs: [string | null, string | null][] = [
+      [null, 'image'], ['', 'image'], ['2026-01-02T08:00:00', 'image'], ['2026-01-02T08:00:00', 'video'],
+      [null, 'image'], ['', null], [null, 'image'],
+    ];
+    const gids: number[] = [];
+    specs.forEach(([taken, mt], i) => {
+      const pid = Number(
+        tp.run('local', null, owner.id, `f${i}.jpg`, i % 2 ? null : `t${i}.jpg`, 100, i % 3 ? null : 50, mt ?? 'image', taken, i % 2 ? 1.5 : null, i % 2 ? 2.5 : null).lastInsertRowid,
+      );
+      gids.push(Number(gp.run(journey.id, pid, i % 2 ? `cap${i}` : null, i % 2, i % 3, null, null, null, 1700000000000 + (i % 4) * 1000).lastInsertRowid));
+    });
+    jep.run(e1, gids[0], 0, now); jep.run(e1, gids[1], 0, now); jep.run(e1, gids[2], 0, now);
+    jep.run(e2, gids[0], 1, now); jep.run(e2, gids[3], 0, now);
+    jep.run(e4, gids[5], 2, now); jep.run(e4, gids[4], 2, now);
+    jep.run(e3, gids[6], 0, now);
+    // An unrelated journey's photo — must never leak into this journey's reads.
+    const other = createJourney(testDb, member.id);
+    const opid = Number(tp.run('local', null, member.id, 'o.jpg', null, null, null, 'image', null, null, null).lastInsertRowid);
+    gp.run(other.id, opid, null, 0, 0, null, null, null, 1);
+
+    return { owner, member, journey };
+  }
+
+  const LEG = {
+    JG15: `SELECT gp.id, jep.entry_id, gp.photo_id, gp.caption, jep.sort_order, gp.shared, gp.created_at,
+      tp.provider, tp.asset_id, tp.owner_id, tp.file_path, tp.thumbnail_path, tp.width, tp.height,
+      tp.media_type, tp.duration_ms, tp.taken_at, tp.lat, tp.lng FROM journey_entry_photos jep
+      JOIN journey_photos gp ON gp.id  = jep.journey_photo_id
+      JOIN trek_photos    tp ON tp.id  = gp.photo_id WHERE jep.entry_id IN (SELECT id FROM journey_entries WHERE journey_id = ?) ORDER BY jep.sort_order ASC`,
+    JG19: `SELECT gp.id, gp.journey_id, gp.photo_id, gp.caption, gp.shared, gp.sort_order, gp.created_at,
+      tp.provider, tp.asset_id, tp.owner_id, tp.file_path, tp.thumbnail_path, tp.width, tp.height,
+      tp.media_type, tp.duration_ms, tp.taken_at, tp.lat, tp.lng FROM journey_photos gp JOIN trek_photos tp ON tp.id = gp.photo_id WHERE gp.journey_id = ? ${GALLERY_CHRONOLOGICAL_ORDER}`,
+    STATS_ENTRIES: `SELECT id, title, location_name, location_lat, location_lng, entry_date, source_trip_id, source_place_id, stats_excluded FROM journey_entries WHERE journey_id = ? AND dismissed = 0 ORDER BY entry_date ASC, sort_order ASC, id ASC`,
+    STATS_TRIPS: `SELECT t.id, t.title, t.start_date AS start, t.end_date AS end FROM journey_trips jt JOIN trips t ON t.id = jt.trip_id WHERE jt.journey_id = ? ORDER BY t.start_date IS NULL, t.start_date ASC, t.id ASC`,
+    STATS_PLACES: `SELECT p.id, p.name, p.lat, p.lng, p.trip_id AS tripId, MIN(d.date) AS day, MIN(da.order_index) AS ord FROM journey_trips jt JOIN places p ON p.trip_id = jt.trip_id LEFT JOIN day_assignments da ON da.place_id = p.id LEFT JOIN days d ON d.id = da.day_id WHERE jt.journey_id = ? GROUP BY p.id ORDER BY day IS NULL, day ASC, ord ASC, p.id ASC`,
+    STATS_PLACECOUNT: `SELECT COUNT(*) AS n FROM journey_trips jt JOIN places p ON p.trip_id = jt.trip_id WHERE jt.journey_id = ?`,
+    STATS_PHOTOCOUNT: `SELECT COUNT(*) AS n FROM journey_photos WHERE journey_id = ?`,
+    STATS_EPHOTOS: `SELECT jep.entry_id AS entryId, gp.photo_id AS photoId FROM journey_entry_photos jep JOIN journey_photos gp ON gp.id = jep.journey_photo_id JOIN trek_photos tp ON tp.id = gp.photo_id WHERE gp.journey_id = ? AND (tp.media_type IS NULL OR tp.media_type = 'image') ORDER BY jep.entry_id ASC, jep.sort_order ASC, gp.sort_order ASC, gp.id ASC`,
+    TRACKS: `SELECT DISTINCT p.id AS place_id, p.trip_id, p.name, p.route_color, p.route_geometry FROM journey_entries je JOIN places p ON p.trip_id = je.source_trip_id WHERE je.journey_id = ? AND je.source_trip_id IS NOT NULL AND p.route_geometry IS NOT NULL ORDER BY p.trip_id, p.id`,
+    SUGG: `SELECT t.id, t.title, t.start_date, t.end_date, t.cover_image, (SELECT COUNT(*) FROM places p INNER JOIN day_assignments da ON da.place_id = p.id WHERE p.trip_id = t.id) as place_count FROM trips t LEFT JOIN trip_members tm ON t.id = tm.trip_id AND tm.user_id = ? WHERE (t.user_id = ? OR tm.user_id = ?) AND t.end_date IS NOT NULL AND t.end_date >= ? AND t.end_date <= date('now') AND t.id NOT IN (SELECT trip_id FROM journey_trips) ORDER BY t.end_date DESC`,
+    PICKER: `SELECT t.id, t.title, t.start_date, t.end_date, t.cover_image, (SELECT COUNT(*) FROM places p INNER JOIN day_assignments da ON da.place_id = p.id WHERE p.trip_id = t.id) as place_count FROM trips t LEFT JOIN trip_members tm ON t.id = tm.trip_id AND tm.user_id = ? WHERE t.user_id = ? OR tm.user_id = ? ORDER BY t.start_date DESC`,
+  };
+
+  /** L8 — JG15's flat row order can differ from legacy on `sort_order` ties across entries (join order vs. the IN subquery); the only consumer groups by entry, so the parity contract is the GROUPED shape, not the flat row order. */
+  function groupedByEntry(rows: { entry_id: number; id: number }[]) {
+    const out: Record<number, number[]> = {};
+    for (const r of rows) (out[r.entry_id] ||= []).push(r.id);
+    return out;
+  }
+
+  it('JourneyEntryPhotosRepository.listForJourney (JG15) matches the legacy statement, grouped by entry (L8: flat order may tie-break differently)', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = testDb.prepare(LEG.JG15).all(journey.id) as { entry_id: number; id: number }[];
+    const converted = (await entryPhotosRepoDirect.listForJourney(journey.id)) as unknown as { entry_id: number; id: number }[];
+
+    expect(groupedByEntry(converted)).toEqual(groupedByEntry(legacy));
+    // Full-key equality still holds per row (only ORDER may tie-differ) — a
+    // sorted-by-id comparison catches a dropped/renamed column.
+    const sortById = (rows: { id: number }[]) => [...rows].sort((a, b) => a.id - b.id);
+    expect(sortById(converted)).toEqual(sortById(legacy));
+  });
+
+  it('JourneyPhotosRepository.galleryRead (JG19, GALLERY_CHRONOLOGICAL_ORDER) matches the legacy statement exactly, including row order', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = testDb.prepare(LEG.JG19).all(journey.id);
+    const converted = await photosRepoDirect.galleryRead(journey.id);
+    expect(converted).toEqual(legacy);
+  });
+
+  it('JourneyEntriesRepository.listStatsRows (JG64) matches the legacy statement', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = testDb.prepare(LEG.STATS_ENTRIES).all(journey.id);
+    expect(await entriesRepoDirect.listStatsRows(journey.id)).toEqual(legacy);
+  });
+
+  it('JourneyEntriesRepository.listStatsTrips (JG65) matches the legacy statement, undated trips sorted last', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = testDb.prepare(LEG.STATS_TRIPS).all(journey.id);
+    expect(await entriesRepoDirect.listStatsTrips(journey.id)).toEqual(legacy);
+  });
+
+  it('JourneyEntriesRepository.listStatsPlaces (JG66) matches the legacy statement, one row per place at its earliest day', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = testDb.prepare(LEG.STATS_PLACES).all(journey.id);
+    expect(await entriesRepoDirect.listStatsPlaces(journey.id)).toEqual(legacy);
+  });
+
+  it('JourneyEntriesRepository.countStatsPlaces (JG67) matches the legacy count', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = (testDb.prepare(LEG.STATS_PLACECOUNT).get(journey.id) as { n: number }).n;
+    expect(await entriesRepoDirect.countStatsPlaces(journey.id)).toBe(legacy);
+  });
+
+  it('JourneyPhotosRepository.countForJourney (JG68) matches the legacy count', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = (testDb.prepare(LEG.STATS_PHOTOCOUNT).get(journey.id) as { n: number }).n;
+    expect(await photosRepoDirect.countForJourney(journey.id)).toBe(legacy);
+  });
+
+  it('JourneyEntryPhotosRepository.listFirstPhotoPerEntry (JG69) matches the legacy statement, videos excluded', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = testDb.prepare(LEG.STATS_EPHOTOS).all(journey.id);
+    expect(await entryPhotosRepoDirect.listFirstPhotoPerEntry(journey.id)).toEqual(legacy);
+  });
+
+  it('JourneyEntriesRepository.listTracksSource (JG63) matches the legacy statement', async () => {
+    const { journey } = await seedParityJourney();
+    const legacy = testDb.prepare(LEG.TRACKS).all(journey.id);
+    expect(await entriesRepoDirect.listTracksSource(journey.id)).toEqual(legacy);
+  });
+
+  it('JourneyEntriesRepository.listSuggestedTrips (JG120) matches the legacy statement', async () => {
+    const { owner } = await seedParityJourney();
+    const since = new Date(Date.now() - 30 * 86400000).toISOString().split('T')[0];
+    const legacy = testDb.prepare(LEG.SUGG).all(owner.id, owner.id, owner.id, since);
+    expect(await entriesRepoDirect.listSuggestedTrips(owner.id, since, todayUtc())).toEqual(legacy);
+  });
+
+  it('JourneyEntriesRepository.listUserTripsPicker (JG121) matches the legacy statement — owner and a member', async () => {
+    const { owner, member } = await seedParityJourney();
+    const legacyOwner = testDb.prepare(LEG.PICKER).all(owner.id, owner.id, owner.id);
+    const legacyMember = testDb.prepare(LEG.PICKER).all(member.id, member.id, member.id);
+    expect(await entriesRepoDirect.listUserTripsPicker(owner.id)).toEqual(legacyOwner);
+    expect(await entriesRepoDirect.listUserTripsPicker(member.id)).toEqual(legacyMember);
+  });
+});
+
 describe('Plan 3g Task 1 — mutation proofs (R5/R7)', () => {
  it("JG119: JourneyContributorsRepository.deleteNonOwner never removes the owner row, even targeted at the owner's own id", async () => {
    const { user: owner } = createUser(testDb);
@@ -81,6 +81,9 @@ beforeAll(async () => {
    // already-built JourneysRepository (JS8/JS12), same instance the domain
    // service above uses.
    await createTestJourneyShareTokensRepo(testDb), journeysRepo,
+    // task-5-fix-brief constructor-ripple: `UnitOfWork` (L1's transactional
+    // create/update) + `JourneyPhotosRepository` (L2's `galleryRead` reuse).
+    uow, await createTestJourneyPhotosRepo(testDb),
  );
 });

@@ -249,6 +252,32 @@ describe('createOrUpdateJourneyShareLink', () => {

    expect(r1.token).not.toBe(r2.token);
  });
+
+  // L1 (task-5-review.md) — two concurrent FIRST creates for the same
+  // journey used to race the existing-link read against the insert: the
+  // loser hit `UNIQUE(journey_id)` and threw (a 500 over HTTP) instead of
+  // the base's `{created:false}`. Races two real `Promise.all` callers
+  // through the actual service on the shared connection.
+  it('JOURNEY-SHARE-L1: two concurrent first-creates for the same journey — one created, one {created:false}, no throw', async () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+
+    const [a, b] = await Promise.all([
+      svc.createOrUpdateJourneyShareLink(journey.id, user.id, { share_timeline: true }),
+      svc.createOrUpdateJourneyShareLink(journey.id, user.id, { share_timeline: false }),
+    ]);
+
+    const results = [a, b];
+    const created = results.filter((r) => r!.created);
+    const notCreated = results.filter((r) => !r!.created);
+    expect(created).toHaveLength(1);
+    expect(notCreated).toHaveLength(1);
+    // Both callers agree on the one token that exists.
+    expect(a!.token).toBe(b!.token);
+
+    const rows = testDb.prepare('SELECT COUNT(*) AS n FROM journey_share_tokens WHERE journey_id = ?').get(journey.id) as { n: number };
+    expect(rows.n).toBe(1);
+  });
 });

 describe('getJourneyShareLink', () => {
@@ -441,6 +470,26 @@ describe('validateShareTokenForAsset', () => {

    expect(result).toBeNull();
  });
+
+  // M5a (task-5-review.md) — JOURNEY-SHARE-015 above only tries an asset id
+  // that does not exist at all; it stays green even with `gp.journey_id = ?`
+  // dropped from `findAssetForValidation` (mutation M23), because the join
+  // still fails to find a row. This test uses an asset id that IS real and
+  // gallery-linked, just to a DIFFERENT journey, so a dropped scope check
+  // would resolve it (leaking a cross-journey asset through a valid token)
+  // where JOURNEY-SHARE-015 cannot catch that.
+  it('JOURNEY-SHARE-M5A: denies an asset that is real but belongs to a DIFFERENT journey (findAssetForValidation journey scope)', async () => {
+    const { user } = createUser(testDb);
+    const sharedJourney = createJourney(testDb, user.id);
+    const otherJourney = createJourney(testDb, user.id);
+    const otherEntry = createJourneyEntry(testDb, otherJourney.id, user.id);
+    insertJourneyPhoto(otherEntry.id, { assetId: 'other-journey-asset', ownerId: user.id });
+    const { token } = await svc.createOrUpdateJourneyShareLink(sharedJourney.id, user.id, {});
+
+    const result = await svc.validateShareTokenForAsset(token, 'other-journey-asset');
+
+    expect(result).toBeNull();
+  });
 });

 describe('getPublicJourney', () => {
@@ -726,9 +775,14 @@ describe('getPublicJourney', () => {

 describe('parity — JourneyShareTokensRepository reads match the legacy statement run raw', () => {
  let shareTokensRepo: JourneyShareTokensRepository;
+  // L2 — JS15 moved to `JourneyPhotosRepository.galleryRead`; P06 below now
+  // pins that method's output against the legacy statement instead of the
+  // (now-deleted) `JourneyShareTokensRepository.listGalleryForPublicJourney`.
+  let journeyPhotosRepo: Awaited<ReturnType<typeof createTestJourneyPhotosRepo>>;

  beforeAll(async () => {
    shareTokensRepo = await createTestJourneyShareTokensRepo(testDb);
+    journeyPhotosRepo = await createTestJourneyPhotosRepo(testDb);
  });

  it('JOURNEY-SHARE-P01: findFlagsByJourneyId (JS1) matches the legacy 5-column read', async () => {
@@ -813,7 +867,7 @@ describe('parity — JourneyShareTokensRepository reads match the legacy stateme
    expect(await shareTokensRepo.listEntryPhotosForPublicJourney(journey.id)).toEqual(legacy);
  });

-  it('JOURNEY-SHARE-P06: listGalleryForPublicJourney (JS15, GALLERY_CHRONOLOGICAL_ORDER) matches the legacy statement — a photo linked to an entry AND an unattached gallery photo', async () => {
+  it('JOURNEY-SHARE-P06: JourneyPhotosRepository.galleryRead (JS15, GALLERY_CHRONOLOGICAL_ORDER) matches the legacy statement — a photo linked to an entry AND an unattached gallery photo', async () => {
    const { user } = createUser(testDb);
    const journey = createJourney(testDb, user.id);
    const entry = createJourneyEntry(testDb, journey.id, user.id, { type: 'entry', entry_date: '2026-01-01' });
@@ -838,7 +892,7 @@ describe('parity — JourneyShareTokensRepository reads match the legacy stateme
      `)
      .all(journey.id);

-    expect(await shareTokensRepo.listGalleryForPublicJourney(journey.id)).toEqual(legacy);
+    expect(await journeyPhotosRepo.galleryRead(journey.id)).toEqual(legacy);
    expect(legacy).toHaveLength(2);
  });

@@ -137,6 +137,20 @@ describe('journeyStats access', () => {
    expect(stats).not.toBeNull();
    expect(stats!.journeyId).toBe(journey.id);
  });
+
+  // M4 (task-5-review.md) — `JourneyEntriesRepository.countStatsPlaces`'s
+  // `row?.n ?? 0` null arm had no test exercising a journey with zero linked
+  // trips at all (worse coverage than base). `places` must still answer 0,
+  // not throw or leave the field undefined.
+  it('places is 0 for a journey with no trips at all (countStatsPlaces)', async () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+
+    const stats = await svc.journeyStats(journey.id, user.id);
+
+    expect(stats).not.toBeNull();
+    expect(stats!.places).toBe(0);
+  });
 });

 describe('journeyStats route', () => {
@@ -429,6 +429,31 @@ describe('canAccessUserPhoto', () => {
    expect(await access.canAccessUserPhoto(viewer.id, owner.id, '0', 'j-asset-viewer', 'immich')).toBe(true);
  });

+  // M5b (task-5-review.md) — mutation M24 dropped `tkp.owner_id = ?` from
+  // `JourneyPhotosRepository.findJourneyIdForAsset` (MA1) and every existing
+  // test stayed green, because none put the SAME asset_id/provider under a
+  // DIFFERENT trek_photos.owner_id than the `ownerUserId` the caller asks
+  // about. Here the asset really is in a journey the requester (the journey
+  // owner) could otherwise reach — but `ownerUserId` names someone who did
+  // NOT upload it, so MA1's owner scope must refuse to resolve it. A dropped
+  // scope would match on asset_id+provider alone, resolve the journey, and
+  // MA2's owner-or-contributor check would then grant access wrongly.
+  it('MEM-ACCESS-M5B: tripId "0" — same asset_id/provider exists in the journey but under a DIFFERENT owner_id (findJourneyIdForAsset owner scope)', async () => {
+    const { user: owner } = createUser(testDb);
+    const { user: realUploader } = createUser(testDb, { username: 'real-uploader-m5b' });
+    const { user: otherOwner } = createUser(testDb, { username: 'other-owner-m5b' });
+    const journeyId = makeJourney(owner.id);
+    const photoId = Number(testDb.prepare(
+      "INSERT INTO trek_photos (provider, asset_id, owner_id) VALUES ('immich', 'm5b-shared-asset', ?)"
+    ).run(realUploader.id).lastInsertRowid);
+    testDb.prepare('INSERT INTO journey_photos (journey_id, photo_id, created_at) VALUES (?, ?, 0)').run(journeyId, photoId);
+
+    // requestingUserId (owner) !== ownerUserId (otherOwner), so the trivial
+    // shortcut does not fire; MA1 must refuse because otherOwner never
+    // uploaded this asset.
+    expect(await access.canAccessUserPhoto(owner.id, otherOwner.id, '0', 'm5b-shared-asset', 'immich')).toBe(false);
+  });
+
  it('MEM-ACCESS-007: tripId "0" refuses someone with no journey link', async () => {
    const { user: owner } = createUser(testDb);
    const { user: stranger } = createUser(testDb, { username: 'stranger' });