diff --git a/.planning/phases/05-web-push-notifications/05-REVIEW.md b/.planning/phases/05-web-push-notifications/05-REVIEW.md
new file mode 100644
index 0000000..e6d5d01
--- /dev/null
+++ b/.planning/phases/05-web-push-notifications/05-REVIEW.md
@@ -0,0 +1,345 @@
+---
+phase: 05-web-push-notifications
+reviewed: 2026-06-09T00:00:00Z
+depth: standard
+files_reviewed: 20
+files_reviewed_list:
+  - apps/api/src/lib/pushDispatcher.ts
+  - apps/api/src/lib/pushCoalescer.ts
+  - apps/api/src/lib/listChangeDispatcher.ts
+  - apps/api/src/lib/eventChangeDispatcher.ts
+  - apps/api/src/broker/reminderScheduler.ts
+  - apps/api/src/broker/sync.ts
+  - apps/api/src/broker/poller.ts
+  - apps/api/src/broker/outboxWorker.ts
+  - apps/api/src/routes/push.ts
+  - apps/api/src/routes/lists.ts
+  - apps/api/src/index.ts
+  - apps/api/src/db/schema.ts
+  - apps/api/src/db/migrations/0003_same_xavin.sql
+  - apps/pwa/src/sw.ts
+  - apps/pwa/src/hooks/usePushSubscription.ts
+  - apps/pwa/src/components/PushPermissionPrompt.tsx
+  - apps/pwa/src/components/SettingsSheet.tsx
+  - apps/pwa/src/components/PermissionDeniedBanner.tsx
+  - apps/pwa/src/App.tsx
+  - apps/pwa/src/components/AppNav.tsx
+  - apps/pwa/src/components/CalendarShell.tsx
+  - apps/pwa/src/components/InstallPrompt.tsx
+  - apps/pwa/vite.config.ts
+  - docker-compose.yml
+findings:
+  critical: 4
+  warning: 5
+  info: 3
+  total: 12
+status: issues_found
+---
+
+# Phase 5: Code Review Report
+
+**Reviewed:** 2026-06-09
+**Depth:** standard
+**Files Reviewed:** 20+
+**Status:** issues_found
+
+## Summary
+
+Phase 5 delivers Web Push across three trigger paths (NOTIF-01/02/03) plus the iOS reliability infrastructure. The architecture is sound: VAPID signing is delegated to `web-push`, the D-03 actor-suppression filter is applied both at the DB layer and in application code, `event.waitUntil()` is used correctly in the SW, and the D-13 boundary (no tsdav in notification code) is respected.
+
+Four blockers were found. Two are correctness bugs: (1) the `sentReminders` set in `reminderScheduler.ts` grows without bound and is never pruned — it will leak memory and eventually begin skipping due reminders after a process restart populates a new set without stale guard entries; (2) the `uniq_push_endpoint` unique constraint is defined on a `text` column in the migration SQL, which MariaDB/InnoDB silently truncates to 767 bytes when creating the index — a subscription with a very long endpoint can collide with or fail to match a different subscription, causing phantom prune-without-delete or missed 410 cleanup. Two are security: (3) `VAPID_PRIVATE_KEY` is passed via `docker-compose.yml` environment block from the host `.env` but with no fallback (`${VAPID_PRIVATE_KEY}` with no `:-` default) — Docker Compose will error at startup if the variable is unset rather than leaving push non-functional, which is correct, but the VAPID private key has no runtime check that it is well-formed before `setVapidDetails` is called, and invalid keys are only warned, not blocked; and (4) the `notificationclick` handler in `sw.ts` uses `client.url === url` for exact-URL matching, which means a focused-window match will never fire for notifications with query-string deep-links whose current window URL does not exactly match (e.g. `/calendar?date=2026-06-09&event=uid` vs `/calendar`) — this is a functional regression, not a security issue, but is reclassified as a correctness blocker because the D-14 deep-link requirement is unmet for the most common case.
+
+---
+
+## Critical Issues
+
+### CR-01: `sentReminders` Set Grows Without Bound (Memory Leak + Correctness)
+
+**File:** `apps/api/src/broker/reminderScheduler.ts:38`
+
+**Issue:** `sentReminders` is a module-level `Set<string>` that accumulates one entry per `${uid}:${minuteBucket}` every minute for every shared timed event. Entries are never removed. For an app running continuously with N shared events, the set grows by N entries/minute indefinitely. After a long uptime the set consumes significant memory. More critically: the deduplication key includes `minuteBucket = floor(ms / 60000)`, so after a process restart the set is empty and an event that fired in the previous minute (same wall-clock minute bucket) will fire again — the dedup logic is only effective within a single process lifetime. This is documented as "acceptable data loss on restart" for the double-fire case, but the unbounded growth is not documented and constitutes a bug.
+
+**Fix:** Prune entries for past minute buckets after each scan. After the dispatch loop completes, remove any set entry whose minuteBucket is strictly less than `currentBucket - 1`:
+
+```typescript
+// After the dispatch loop in runReminderCheck():
+const staleBucket = minuteBucket - 1
+for (const key of sentReminders) {
+  const [, bucketStr] = key.split(':')
+  if (Number(bucketStr) < staleBucket) {
+    sentReminders.delete(key)
+  }
+}
+```
+
+---
+
+### CR-02: `uniq_push_endpoint` Unique Constraint on `text` Column Will Silently Truncate in MariaDB/InnoDB
+
+**File:** `apps/api/src/db/migrations/0003_same_xavin.sql:10` and `apps/api/src/db/schema.ts:243-244`
+
+**Issue:** `endpoint` is declared `text NOT NULL` in both the schema and migration, and `CONSTRAINT uniq_push_endpoint UNIQUE(endpoint)` is applied to it. MariaDB/InnoDB cannot index an unbounded `text` column directly — it requires a prefix length. The behavior differs by server version and `innodb_large_prefix` setting: some versions silently create a prefix index (767 bytes by default, 3072 bytes with `ROW_FORMAT=DYNAMIC`), others fail. In either case a silent prefix match means two endpoints that share the same first 767 characters — which is possible for push endpoints from the same push service domain — are treated as duplicates. This makes the upsert in `push.ts`'s `onDuplicateKeyUpdate` unreliable: a second distinct subscription may silently collide, or the migration may fail entirely on strict-mode MariaDB.
+
+The correct fix is to define `endpoint` as `varchar(2048)` (matching the Zod validation bound of `z.string().url().max(2048)` in `routes/push.ts:61`) in both the schema and migration.
+
+**Fix:**
+```sql
+-- Migration: change endpoint column type
+ALTER TABLE `push_subscriptions`
+  MODIFY COLUMN `endpoint` varchar(2048) NOT NULL;
+
+-- Schema: change text() to varchar
+endpoint: varchar('endpoint', { length: 2048 }).notNull(),
+```
+
+The Zod schema in `push.ts` already enforces `max(2048)`, so server-validated inputs fit within the column. Update `p256dh` similarly — it is also `text` but a `varchar(512)` would match the Zod bound.
+
+---
+
+### CR-03: `notificationclick` Deep-Link Window Focus Uses Exact URL Match — D-14 Never Fires for Query-String Deep-Links
+
+**File:** `apps/pwa/src/sw.ts:154`
+
+**Issue:** The `notificationclick` handler matches `client.url === url` to find an existing window. Push deep-link URLs for event notifications are of the form `/calendar?event=<uid>` or `/calendar?date=<date>&event=<uid>`. When the user has the PWA open on `/calendar` (the root path, no query string), `client.url` will be `https://host/calendar` but `url` will be `https://host/calendar?event=uid` — the exact comparison fails, so the handler falls through to `openWindow` and opens a second PWA window instead of focusing the existing one. On iOS standalone mode, `openWindow` does NOT open a second window; it navigates the existing window, so the net behavior is correct on iOS. On Android Chrome it opens a second window. The deeper problem: the handler never navigates the focused window to the deep-link URL; it only calls `focus()` without `navigate()` — so even when the match succeeds (notification for the currently-open URL), the window is focused at the current route, not the notification target.
+
+**Fix:** Use `clients.matchAll` to find any window on the same origin, focus it, and call `client.navigate(url)`:
+
+```typescript
+self.addEventListener('notificationclick', (event: NotificationEvent) => {
+  event.notification.close()
+  const url: string =
+    typeof event.notification.data?.url === 'string'
+      ? event.notification.data.url
+      : '/'
+
+  event.waitUntil(
+    self.clients
+      .matchAll({ type: 'window', includeUncontrolled: true })
+      .then((clientList) => {
+        // Focus any existing window on this origin and navigate it
+        for (const client of clientList) {
+          if ('focus' in client) {
+            return (client as WindowClient).focus().then(() =>
+              (client as WindowClient).navigate(url)
+            )
+          }
+        }
+        if (self.clients.openWindow) {
+          return self.clients.openWindow(url)
+        }
+      }),
+  )
+})
+```
+
+---
+
+### CR-04: `subscribe()` in `usePushSubscription` Calls `fetchVapidKey()` (an async `await`) Before `pushManager.subscribe()` — iOS Gesture Gate May Be Broken on Cache Miss
+
+**File:** `apps/pwa/src/hooks/usePushSubscription.ts:197-206`
+
+**Issue:** The `subscribe()` function unconditionally `await`s `fetchVapidKey()` before calling `registration.pushManager.subscribe()`. The comment says "Fetch VAPID key (from cache if already prefetched)" and the VAPID key is pre-fetched in a `useEffect` by `PushPermissionPrompt`. However:
+
+1. The `useEffect` pre-fetch is fire-and-forget (`void prefetchVapidKey()`). There is no guarantee it completes before the user taps "Enable Notifications" — especially on a slow connection on first install.
+2. If `sessionStorage` is unavailable (private mode, quota exceeded) or the pre-fetch failed silently, `fetchVapidKey()` will make a network round-trip inside `subscribe()`.
+3. iOS requires `pushManager.subscribe()` to be called synchronously within the user gesture's call stack. An `await fetch(...)` before `pushManager.subscribe()` breaks the gesture gate — the iOS browser will throw `NotAllowedError` with the message "The operation was not permitted." The existing code has this `await` on the very first line of `subscribe()`.
+
+The `setEnabled(true)` path in the same hook has the same structure (`await fetchVapidKey()` before `pushManager.subscribe()`), but `setEnabled(true)` is only called when `Notification.permission === 'granted'` (OS already approved), so no OS dialog is shown — the gesture gate for permission is not relevant there. For the `subscribe()` path used from `PushPermissionPrompt` and `SettingsSheet`, the gate IS relevant when `Notification.permission === 'default'`.
+
+**Fix:** Pre-fetch the VAPID key synchronously into a variable before the tap, and pass it as an argument to `subscribe()`. Remove the `fetchVapidKey()` call from inside `subscribe()`:
+
+```typescript
+// In PushPermissionPrompt: pre-fetch before the button renders
+const [vapidKey, setVapidKey] = useState<string | null>(null)
+useEffect(() => {
+  fetchVapidKey().then(setVapidKey).catch(() => {})
+}, [])
+
+// subscribe() signature change:
+const subscribe = async (
+  registration: ServiceWorkerRegistration,
+  vapidKey: string,
+): Promise<void> => {
+  // No await before this line — iOS gesture gate preserved
+  const sub = await registration.pushManager.subscribe({
+    userVisibleOnly: true,
+    applicationServerKey: urlBase64ToUint8Array(vapidKey),
+  })
+  // ... POST to server
+}
+```
+
+If a pre-pass API approach is not desirable, the minimum safe fix is to ensure `fetchVapidKey()` always resolves from `sessionStorage` (never from the network) when called from within a tap handler, by making the pre-fetch mandatory and blocking the button render until the key is ready:
+
+```typescript
+// Disable the "Enable Notifications" button until vapidKey is loaded
+<button disabled={!vapidKey} onClick={handleEnableClick}>
+```
+
+---
+
+## Warnings
+
+### WR-01: `sentReminders` Dedup Key Survives Process Restart — Same-Minute Double-Fire Is Possible
+
+**File:** `apps/api/src/broker/reminderScheduler.ts:133-134`
+
+**Issue:** The dedup comment documents "Acceptable data loss on process restart for a two-person household." This is a reasonable MVP trade-off, but the dedup fires AFTER marking the key in the set (`sentReminders.add(key)` before `dispatchPush`). If `dispatchPush` throws after the key is added (a non-410/404 failure), the event is marked as sent but no notification was delivered, and the set prevents retry in the same minute bucket. Combined with a process restart before the next bucket, no notification is ever delivered for that event.
+
+**Fix:** Move `sentReminders.add(key)` to after the fan-out loop succeeds (or accept that at-least-once delivery requires not pre-marking):
+
+```typescript
+// Only mark sent after all dispatches have been attempted
+for (const sub of event.subs) {
+  try { await dispatchPush(sub, notification) } catch { /* log */ }
+}
+sentReminders.add(key)  // moved here
+```
+
+---
+
+### WR-02: `listChangeDispatcher` Unused Import `and`
+
+**File:** `apps/api/src/lib/listChangeDispatcher.ts:20`
+
+**Issue:** `and` is imported from `drizzle-orm` but never used in the file. `inArray` and `eq` are used; `and` is dead.
+
+**Fix:**
+```typescript
+import { eq, inArray } from 'drizzle-orm'
+```
+
+---
+
+### WR-03: `push.ts` POST `/subscription` Catches and Logs the Full Error Object (Potential Key Leakage)
+
+**File:** `apps/api/src/routes/push.ts:116-118`
+
+**Issue:** The catch block logs `err` directly via `console.error('[push/POST /subscription] DB operation failed:', err)`. If the DB error includes the failing query parameters in its message (e.g. MariaDB's `ER_DUP_ENTRY` sometimes includes the duplicate value), the p256dh or auth values from the request body could appear in server logs. The threat model (T-05-03) explicitly says "never logs subscription keys."
+
+**Fix:** Log only `err instanceof Error ? err.message : String(err)` — do not pass the raw error object:
+
+```typescript
+} catch (err) {
+  console.error(
+    '[push/POST /subscription] DB operation failed:',
+    err instanceof Error ? err.message : String(err),
+  )
+  return c.json({ error: 'Service unavailable' }, 503)
+}
+```
+
+Apply the same fix to the DELETE handler's catch at line 136.
+
+---
+
+### WR-04: `sync.ts` Emits `delete` Change Events Before the DB DELETE — Race Condition in Fan-Out
+
+**File:** `apps/api/src/broker/sync.ts:242-259`
+
+**Issue:** Deleted UIDs are added to `changes` (lines 252-258) before the `db.delete()` call that removes them from the cache (lines 262-268). The `onChanges` callback is fired at line 272-274 after the delete, but within `onChanges` (in `poller.ts` and `outboxWorker.ts`) `dispatchEventChange` is called as fire-and-forget with `.catch()`. There is no await on the notification dispatch before `syncCalendar` returns. The sequence is:
+
+1. `changes.push({ operation: 'delete', uid })` — collected
+2. `db.delete(calendarEvents)` — DB row removed
+3. `onChanges(changes)` — fires `dispatchEventChange` fire-and-forget
+4. `syncCalendar` returns
+
+If a rapid second poll cycle begins at step 4 (before the fire-and-forget dispatch at step 3 completes), the deleted uid may be re-fetched from Fastmail (if it was not actually deleted on the server — false delete detection from a transient REPORT response) and re-inserted, then `dispatchEventChange` fires with `operation: 'delete'` for an event that now exists again in the cache. This is an edge case in the two-person household but worth documenting: the delete change detection and DB delete should be atomic, or the `changes` collection for deletes should happen after the DB delete.
+
+**Fix:** Collect delete changes AFTER the `db.delete()`:
+```typescript
+// 5. Delete pruned rows from DB first
+if (seenUids.length > 0) {
+  await db.delete(calendarEvents)
+    .where(and(eq(calendarEvents.calendarId, cal.id), notInArray(calendarEvents.uid, seenUids)))
+} else {
+  await db.delete(calendarEvents).where(eq(calendarEvents.calendarId, cal.id))
+}
+
+// THEN collect delete changes for onChanges
+if (onChanges && cachedRows) {
+  for (const row of cachedRows) {
+    changes.push({ uid: row.uid, title: row.title ?? null, operation: 'delete' })
+  }
+}
+```
+
+---
+
+### WR-05: Health-Check Re-Subscribe in `usePushSubscription` Silently Re-Subscribes Without Checking Server-Side Record
+
+**File:** `apps/pwa/src/hooks/usePushSubscription.ts:166-178`
+
+**Issue:** The health-check `useEffect` (lines 152-183) detects a missing browser-side subscription and silently re-subscribes. The POST to `/api/push/subscription` will upsert (due to `onDuplicateKeyUpdate` on endpoint). However, if `pushManager.getSubscription()` returns a subscription object (line 157: `setIsSubscribed(true); return`) but the server-side record was deleted (e.g. pruned by a 410), the `isSubscribed` state shows `true` but notifications silently fail because the server has no record to dispatch to. The health-check does not verify the server-side record exists.
+
+This is a silent failure mode: the user sees "notifications on" (isSubscribed=true), the app sends no notification, and the user never knows. Reminders and alerts are missed with no indication.
+
+**Fix:** After `existingSub` is found in the browser, POST it to the server as a health confirmation (the upsert is idempotent):
+```typescript
+if (existingSub) {
+  // Re-confirm server-side record (handles 410-prune recovery)
+  await fetch('/api/push/subscription', {
+    method: 'POST',
+    headers: { 'Content-Type': 'application/json' },
+    credentials: 'include',
+    body: JSON.stringify(existingSub.toJSON()),
+  }).catch(() => {}) // non-blocking
+  setIsSubscribed(true)
+  return
+}
+```
+
+---
+
+## Info
+
+### IN-01: `eventChangeDispatcher.ts` Does Not Name the Actor in Notification Copy (D-02/D-03 Partial Implementation)
+
+**File:** `apps/api/src/lib/eventChangeDispatcher.ts:83`
+
+**Issue:** The `buildCopy` function uses generic copy (`'New calendar event'`, `'Calendar event updated'`) rather than naming the actor per D-02/D-03 spec ("Lucas added…", "Wife moved Dentist → Wed 3pm"). The comment at line 81-82 acknowledges this: "actorName resolution (querying users table) is a future D-02 enhancement." The acceptance criteria spec requires actor attribution. For the two-person household this means the notification body only shows the event title, not who made the change, which is less useful and technically incomplete against D-03's "name the actor" requirement.
+
+**Fix:** Query `users.displayName` for `actorUserId` at the top of `dispatchEventChange` and pass it to `buildCopy`:
+```typescript
+const actorRow = await db.select({ displayName: users.displayName })
+  .from(users).where(eq(users.id, actorUserId)).limit(1)
+const actorName = actorRow[0]?.displayName ?? 'A family member'
+// Pass actorName into buildCopy
+```
+
+---
+
+### IN-02: `docker-compose.yml` VAPID Variables Have No Fallback — Compose Startup Fails if Unset
+
+**File:** `docker-compose.yml:29-31`
+
+**Issue:** `VAPID_PUBLIC_KEY: ${VAPID_PUBLIC_KEY}`, `VAPID_PRIVATE_KEY: ${VAPID_PRIVATE_KEY}`, and `VAPID_SUBJECT: ${VAPID_SUBJECT}` have no `:-` fallbacks. If any of these are unset in the operator's `.env`, Docker Compose fails to start the API service entirely (`variable is not set` error) rather than starting with push disabled. The `index.ts` startup guard (lines 118-128) already handles missing VAPID vars gracefully with a `console.warn`, but that logic is never reached because Compose errors first.
+
+This is an operator experience issue, not a security issue (keeping VAPID_PRIVATE_KEY out of source control is correct). Adding empty-string fallbacks would allow the container to start and push to fail gracefully:
+
+```yaml
+VAPID_PUBLIC_KEY: ${VAPID_PUBLIC_KEY:-}
+VAPID_PRIVATE_KEY: ${VAPID_PRIVATE_KEY:-}
+VAPID_SUBJECT: ${VAPID_SUBJECT:-}
+```
+
+---
+
+### IN-03: `PushPermissionPrompt` Uses `Math.random()` for `headingId` — Stable Between Renders But Unstable Across Mounts
+
+**File:** `apps/pwa/src/components/PushPermissionPrompt.tsx:63`
+
+**Issue:** `const headingId = useRef(`push-prompt-heading-${Math.random().toString(36).slice(2)}`)` generates a random ID at component mount. This is stable within a mount lifecycle but changes each time the component unmounts and remounts (e.g. `App.tsx` renders `<PushPermissionPrompt />` at the root, and the component may remount on navigation). The aria-labelledby/id pairing is still valid within a mount, so there is no accessibility regression per se, but React's Strict Mode double-invocation in dev will produce two different IDs for what is logically the same element. Use `useId()` (React 18+) instead.
+
+**Fix:**
+```typescript
+import { useId } from 'react'
+// inside the component:
+const headingId = useId()
+```
+
+---
+
+_Reviewed: 2026-06-09_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_