**Method:** 19 parallel audit agents on Claude Opus 4.7, read-only static analysis. Each agent owned a single domain and wrote a CRITICAL/HIGH/MEDIUM-grouped report. This document consolidates the reports and overlays the fixes already shipped during the session.
---
## How to read this document
1.**Executive summary** lists every CRITICAL finding (must address before production), per domain.
2.**Already fixed in this session** is a manifest of the changes I shipped while the audit was running. Don't re-fix these.
3.**Cross-cutting priority queue** is the top ~15 highest-impact findings across the entire codebase, ordered. Tackle these first.
4.**Per-domain reports** below contain the full text of every agent's report verbatim — useful when you sit down to actually fix a specific area.
5.**Methodology + agent roster** appendix at the bottom lists who looked at what.
Severity is the auditor's judgment, not mine — I have not re-graded findings. Treat anything tagged CRITICAL as a real block on shipping.
| 1 | Security | `src/app/api/v1/admin/users/[id]/permission-overrides/route.ts` | Admins could grant themselves every permission leaf via self-target | **FIXED this session** |
| 2 | Security | `src/app/api/auth/resolve-identifier/route.ts` | Username enumeration via hit/miss response shape + no rate limit | **FIXED this session** |
| 3 | Services | `src/lib/services/users.service.ts` (admin email-change) | `account.accountId` not updated → user can't sign in with either old or new email after admin rotation; sessions also not revoked | **FIXED this session** |
| 4 | Observability | `src/lib/services/search-nav-catalog.ts` | 10 NAV_CATALOG entries pointed at routes that don't exist (`/admin/audit-log`, `/admin/error-events`, `/user-settings`, 7×`/settings/<x>`) | **FIXED this session** |
| 6 | Email | `src/lib/env.ts` + `src/lib/email/index.ts` | `EMAIL_REDIRECT_TO` has no `NODE_ENV=production` guard — a stray prod env value silently funnels every email to one inbox | Open |
| 7 | Email | every template | URL interpolations into `href="…"` and link text are unescaped — a `"` in any URL breaks out, no scheme rejection | Open |
| 8 | Data model | `src/lib/db/migrations/0052_audit_critical_fixes.sql` | `CREATE INDEX CONCURRENTLY` silently never runs because there's no real `db:migrate` runner — six composite indexes missing in prod | Open |
| 9 | Data model | `db:push` flow | Two structural constraints (berths.current_pdf_version_id circular FK, system_settings NULLS NOT DISTINCT) not in `db:push`; fresh-deploy diverges from prod | Open |
| 10 | Services | `documents.service.ts: handleDocumentCompleted` | Orphan-blob window — failure between `storage.put` and `documents.update` leaves the blob and marks status='completed' with no `signedFileId` | Open |
| 12 | GDPR | `src/lib/services/client-hard-delete.service.ts` | "Right to be forgotten" doesn't actually erase — verbatim PII survives in email_messages.body_html, files, document_sends.recipient_email forever | Open |
| 13 | GDPR | `src/app/api/auth/resolve-identifier/route.ts` (post-fix) | Still echoes the real canonical email on a successful username hit (rate-limited but enumerable) | Partial — see Open follow-ups |
| 14 | GDPR | `audit_logs.metadata` field | Not covered by `maskSensitiveFields`; raw PII (emails, IPs, names) accumulates unbounded with no retention cron | Open |
| 15 | Observability | `src/app/api/webhooks/documenso/route.ts` | Webhook handler bypasses the platform-error pipeline entirely — admin/errors silent on Documenso webhook crashes | Open |
| 16 | UI/UX | 16 sites use native `window.confirm()` | Bypasses `ConfirmationDialog` / `AlertDialog` for destructive flows (cancel signing, delete files, archive interest/company/yacht…) | Open |
| 17 | Documenso | `documenso-client.ts` v1↔v2 routing | (Pending full report) | In progress |
| 18 | Concurrency | (see report) | Various race windows on multi-rep edits + partial-unique-index inserts | Open |
- **Self-target privilege escalation block** — `src/app/api/v1/admin/users/[id]/permission-overrides/route.ts` now refuses `PUT` when `targetUserId === ctx.userId`. Additionally, the body now sanitises against a canonical `ALLOWED_RESOURCE_ACTIONS` allow-list mirroring `RolePermissions`, so unknown resource/action keys are stripped before write. Cross-tenant pollution check added (refuses overrides for users without a `user_port_roles` row in the caller's port).
- **Username enumeration kill** — `src/app/api/auth/resolve-identifier/route.ts` now (a) shares the `auth` 5-per-15-min rate-limit bucket keyed by client IP, (b) returns a synthetic `@auth.invalid` email on miss so hit and miss are indistinguishable in shape. (Note: GDPR auditor flagged the hit-path still echoes a real canonical email — still an information leak that's worth a deeper redesign; see Open follow-ups.)
- **Email-change account/session rotation** — `src/lib/services/users.service.ts` now also updates `account.accountId` for the `credential` provider (Better Auth's actual login key) AND revokes every active `session` row when an admin rotates a user's email. Previously the user could not sign in with either old or new email after rotation.
- **Middleware unblocks token-gated email routes** — `src/middleware.ts` adds `/api/v1/me/email/confirm/` and `/api/v1/me/email/cancel/` to `PUBLIC_PATHS` so the confirm/cancel links work in a fresh browser without an existing session.
- **NAV_CATALOG dead-link sweep** — `src/lib/services/search-nav-catalog.ts` corrected 10 entries that pointed to non-existent routes. `/admin/audit-log` → `/admin/audit`, `/admin/error-events` → `/admin/errors`, `/user-settings` → `/settings/profile`, and the 7 phantom `/settings/<x>` entries redirected to their real `/admin/<x>` homes.
- **Topbar global search extended** — every admin sub-card now indexed in `NAV_CATALOG` with curated `keywords` (client portal, ai scoring, pipeline weights, recommender heat weights, etc.). Results sort to the bottom of the cmd-K dropdown, beneath entity hits.
- **Admin sections page search** — `src/components/admin/admin-sections-browser.tsx``AdminSection` gained a `keywords?: string[]` field, populated for System Settings (mirrors `KNOWN_SETTINGS`), AI configuration, OCR, Users, and Website analytics. `filteredMatches` haystack now includes those keywords.
- **Disable / enable button** — third Power/PowerOff action button on the desktop user list + matching dropdown item on the mobile card. Backed by `userProfiles.isActive` (already enforced by `withAuth` → 403 on disabled accounts).
- **UserForm tabs + permissions matrix** — UserForm now wraps Profile & role + Permissions in tabs. New `UserPermissionMatrix` component renders the full `RolePermissions` shape with three-state per-leaf toggle (Inherit / Grant / Deny). The matrix is `role="radiogroup"` + `aria-checked` per option, and shows an amber callout explaining that overrides save on their own button. Dirty-state tracked via originalOverrides comparison.
- **First/last name + admin email change** — UserForm collects first + last name (canonical) alongside displayName. Email change behind an AlertDialog confirmation; on confirm sends an automated notice to the prior address (new template `src/lib/email/templates/admin-email-change.ts`).
- **Phone formatting** — UserForm swaps the bare tel input for the shared `PhoneInput` (country combobox + AsYouType + E.164 storage).
-`formatRole()` + `ROLE_LABELS` in `src/lib/constants.ts` — replaces the ad-hoc `humanizeRole` in `sidebar.tsx` and `prettifyRoleName` in `role-list.tsx`. user-list, user-card, role-list, user-form now render "Sales Agent" instead of "sales_agent".
- Saved feedback memory: "Be thorough — audit everything that ends in a user-facing notification". (Memory subsystem is /Users/matt/.claude/projects/...)
---
## Cross-cutting priority queue
Tackle in this order. C-prefix = CRITICAL still open; H-prefix = HIGH.
1.**[C] Wire a real `db:migrate` runner** — without it, `0052_audit_critical_fixes.sql` silently never creates 6 composite indexes (data-model C1). Recommended: a tsx script that reads migrations in order, splits on `--> statement-breakpoint`, runs `CREATE INDEX CONCURRENTLY` outside a tx, and tracks state in a `__drizzle_migrations` table. Same script gives you `db:migrate:status` for prod readiness.
2.**[C] Add `EMAIL_REDIRECT_TO` prod guard** — `src/lib/env.ts` should refine to reject when `NODE_ENV === 'production'`, and `src/lib/email/index.ts` should `logger.warn` at boot when set (not debug). 5 minutes of work, prevents an extremely-bad-day class of incident.
3.**[C] Fix orphan-blob window in `handleDocumentCompleted`** — `src/lib/services/documents.service.ts:1100-1253`. Wrap the storage.put + files.insert + documents.update sequence in a transaction or a saga with a compensating delete. The current catch-block path also incorrectly marks `status='completed'` with no `signedFileId`, hiding the failure from reps.
4.**[C] Escape URLs in email templates** — every template in `src/lib/email/templates/*` inlines `${data.link}` etc. into href/text without escaping. Move all template rendering through a shared `escapeUrl` helper and add scheme allow-listing (http(s) only).
5.**[C] Eliminate the 16 native `window.confirm()` calls** — each one is a destructive flow that bypasses `ConfirmationDialog` / `AlertDialog`. ui-ux-auditor lists the sites; high-leverage UX fix.
6.**[C] GDPR export completeness** — `gdpr-bundle-builder.ts` must include portal_users, email_threads/messages, document_sends, reminders, files, scratchpadNotes, client_merge_log, contact_log, website_submissions, form_submissions. This is a regulator-finding-level gap.
7.**[C] Right-to-be-forgotten actually erase** — `client-hard-delete.service.ts` currently nullifies FKs but leaves verbatim PII in email_messages.body_html, files, document_sends.recipient_email. Add a true wipe path (or document the limitation in the legal text and gate the feature behind a "we cannot fully erase X" warning).
8.**[C] Add `user_permission_overrides.user_id` FK + onDelete='set null' on nullable client refs** — data-model H1+H2. Migration 0056.
9.**[C] Resolve-identifier hit-path still leaks email** — replace the API entirely with a server-side signIn proxy that takes `{identifier, password}` and never returns the canonical email at all. Current rate-limited hit still echoes real emails to anyone with a guessable username.
10.**[H] Re-audit `audit_logs.metadata` masking** — extend `maskSensitiveFields` to cover `audit_logs.metadata`; add a 90-day retention cron (mirroring `error_events`).
11.**[H] Webhook → error pipeline** — `documenso/route.ts` should `captureErrorEvent` on handler crash. Apply the same to every other webhook route.
12.**[H] Wire admin email-template subject editor** — 5 of 8 templates ignore `overrides.subject`; admins see "Saved" with zero effect. `email-auditor` H1+H2.
13.**[H] Wire admin signature/footer fields** — `/admin/email` writes `email_signature_html` + `email_footer_html` which the shell never reads. Either delete or wire.
14.**[H] PII redaction in audit/error pipeline** — `error_events.request_body_excerpt` sanitizer redacts password/token but not email/phone/name/dob/address.
15.**[H] Notification email worker XSS** — `workers/notifications.ts:65-71` interpolates `notif.description` and `notif.link` into HTML unescaped. Apply `escapeHtml` + URL allow-list.
---
## Per-domain reports
Each section below is the agent's report verbatim. File:line refs reference the repo as it stands at the start of the audit session — some have already been addressed (see "Already fixed in this session" above).
---
## 1. Security + API + auth audit (security-auditor + early api-security run)
Two reports — the team-spawned `security-auditor` and an earlier standalone run. Both included verbatim.
### Report A: security-auditor (team)
# Security / API / Auth Audit — `feat/documents-folders` branch
Read-only audit of the `pn-crm` repo. Scope: auth wrappers, tenant scoping,
public/webhook endpoints, the just-shipped username-resolve + permission-
`updateOverridesSchema` is `z.record(z.string(), z.record(z.string(), z.boolean()))` — no allow-list against the known `RolePermissions` resource/action keys. An admin (or a stolen admin session) can persist arbitrary keys into `user_permission_overrides.permission_overrides`. Two concrete impacts: (a) future deep-merge logic that maps unknown keys into newly added resources promotes the rogue keys silently (silent privilege creep when new permissions ship); (b) the JSONB can be bloated to harm downstream readers. **Fix:** validate against `KNOWN_PERMISSION_LEAVES` derived from `RolePermissions` (resource → action set), reject unknown keys with `ValidationError`, and bound the merged blob size as `/api/v1/me/route.ts` already does for `preferences`. The GET handler is fine — it only reads what was already persisted.
### H4. `/api/v1/me/email/confirm|cancel/[token]` is unreachable for logged-out users (middleware 401)
**Why it matters:** native confirm cannot be styled, bypasses our `<AlertDialog>` keyboard semantics, no focus trap, no destructive-action red styling, fails focus-return after dismiss; inconsistent with the rest of the app which uses `ConfirmationDialog`. Several of these are catastrophic (cancel signing envelope, hard-delete file, archive company).
**Fix:** replace each with `<ConfirmationDialog destructive title=… description=… onConfirm={…}>` matching the pattern in `user-list.tsx`.
`src/components/admin/users/user-form.tsx:204-212` and `user-permission-matrix.tsx:175-191`.
The matrix has its own "Save overrides" button; the parent Sheet's "Save changes" only persists Profile-tab fields. `onSaveStateChange` is declared in the matrix props but **never passed** by `user-form.tsx` (line 206), so the parent has no idea overrides are dirty. A user who toggles Inherit/Grant/Deny then clicks "Save changes" loses everything when the Sheet closes — no warning, no toast.
**Fix:** lift `overrides` state to `user-form.tsx`, persist both endpoints inside `persist()`, or track dirty state via `onSaveStateChange` and block Sheet close with an AlertDialog.
---
## HIGH
### H1 — Raw enum render via `.replace(/_/g, ' ')` outside `constants.ts` (40+ sites)
`src/components/settings/user-settings.tsx:69-92` — `loadProfile()` reads `firstName`, `lastName`, `email`, etc., but **never reads `phone`** into state. Yet `saveProfile()` at line 143 sends `phone: phone || null`, which **clears the user's stored phone on every save**. Also `country as never` cast at line 298 is unsound — when no country is selected the PhoneInput shows a US flag even for European users.
**Fix:** add `phone` to MeResponse + `setPhone(res.data.profile?.phone ?? '')`. Store country alongside phone (the PhoneInput value is `{e164, country}` — persist the parsed country).
### H4 — UserPermissionMatrix three-state toggle has no a11y semantics
`user-permission-matrix.tsx:247-267` — three sibling `<button>` elements with no `role="radiogroup"`/`role="radio"`/`aria-checked`. Screen readers announce "button, Grant" with no indication which is selected, what the options are, or that they're mutually exclusive. Also no focus ring on the active option.
**Fix:** wrap in `<div role="radiogroup" aria-label={`${action} permission`}>` and set `role="radio" aria-checked={state === opt}` on each. Or use Radix `RadioGroup` for keyboard arrow navigation.
### H5 — Login page form errors not associated with inputs
`src/app/(auth)/login/page.tsx:84-119` — `<p className="text-sm text-destructive">{errors.identifier.message}</p>` is rendered after the input but the `<Input>` has no `aria-describedby` pointing at it, and no `aria-invalid={!!errors.identifier}`. Same for password. Screen readers won't read the error message when focus lands on the input.
**Fix:** give each error `<p id="identifier-error">`, add `aria-describedby={errors.identifier ? 'identifier-error' : undefined}` and `aria-invalid={!!errors.identifier}` on the Input.
### H6 — Desktop sidebar nav lacks `aria-current="page"`
`src/components/layout/sidebar.tsx:177-201` (`NavItemLink`) — uses `active` for visual styling but doesn't set `aria-current` on the `<Link>`. Mobile bottom tabs already do this (`mobile-bottom-tabs.tsx:85`). Screen-reader users cannot identify the current page in the desktop sidebar.
**Fix:** `aria-current={active ? 'page' : undefined}` on the `<Link>`.
---
## MEDIUM
### M1 — Berth status pills use ad-hoc Tailwind colors instead of `StatusPill`
`src/components/berths/berth-columns.tsx:117-119`, `berth-card.tsx:21-23`, `berth-detail-header.tsx:90` — `bg-green-100 text-green-800`, `bg-yellow-100`, `bg-red-100`. The codebase has `StatusPill` (`src/components/ui/status-pill.tsx`) with semantic tokens (success-bg, warning-bg, error-bg) already used by docs/reservations. Berth statuses (available/under_offer/sold) map cleanly to active/expired/rejected pill states.
**Fix:** replace ad-hoc badges with `<StatusPill status={…}>` and extend `statusPillVariants` if a new tone is needed.
### M2 — UserList "Active"/"Disabled" badge inconsistent with StatusPill convention
`src/components/admin/users/user-list.tsx:104-115` — uses `<Badge variant="default" className="bg-green-600">` with inline green override and `<Badge variant="destructive">`. The Power/PowerOff icons and ShieldCheck/ShieldOff icons (also row 107/112) lack `aria-hidden` — but text is present so it's not blocking, just inconsistent.
**Fix:** use `<StatusPill status="active">Active</StatusPill>` / `<StatusPill status="archived">Disabled</StatusPill>`; add `aria-hidden` to all decorative `lucide-react` icons in the table.
### M3 — Only 5 of 73 dashboard routes have a `loading.tsx`
Only `clients/[clientId]`, `invoices`, `expenses`, `admin/errors`, `admin/errors/[requestId]` have route-level loading skeletons. The rest fall back to a blank flash. Lists/details that fetch via React Query show a skeleton inside the component, but full-page navigations show nothing.
**Fix:** add `loading.tsx` per route segment that returns a `<Skeleton>` matching the page chrome (sidebar/topbar already render via the layout).
### M4 — UserPermissionMatrix loading state uses text, not Skeleton
`user-permission-matrix.tsx:193-197` renders `"Loading permissions…"` text. Other list/detail loaders in the app use `<Skeleton>` from `@/components/ui/skeleton`. Adds inconsistency.
**Fix:** replace with a Skeleton grid mirroring the accordion shape.
`user-settings.tsx` lines 167, 184, 197 (`usernameMsg`, `emailMsg`, `resetMsg`) — these `useState` strings stay rendered as a `<span>` next to their button indefinitely. Login uses `toast.error()`; reset-password and other auth surfaces also use sonner.
**Fix:** swap to `toast.success()` / `toast.error()`. Removes stale messages and the inconsistency between auth and settings.
### M6 — Email-or-username Login input: visible placeholder collides with sr-only space
`src/app/(auth)/login/page.tsx:93` — `placeholder="you@example.com or yourname"` with two literal spaces. Mac VoiceOver reads "you at example dot com or yourname" — fine; but the double space is just sloppy formatting. Also the placeholder duplicates the Label "Email or username" — placeholder is unreliable for instructions (clears on focus).
**Fix:** single-space the placeholder, or move the format hint into a `<p id="identifier-hint" className="text-xs text-muted-foreground">` and wire `aria-describedby`.
### M7 — User settings username card: client-side pattern validation never surfaces inline
`user-settings.tsx:359-386` — `pattern="^[a-z0-9._-]{2,30}$"` on the input. HTML5 validation only fires on form submit (this isn't inside a `<form>`); the Save button is a plain `<Button onClick>`. So invalid input only fails server-side with a generic 400. No `aria-describedby` pointing at the helper text (line 382-385).
**Fix:** add a zod-resolved react-hook-form mini-form OR validate on blur and show inline error; wire `aria-describedby="username-help"`.
### M8 — UserForm Tabs: focus does not follow tab switch & no dirty-tab indicator
`user-form.tsx:194-212` — switching from Profile to Permissions doesn't move keyboard focus to the matrix; switching back loses scroll position. The Permissions tab trigger is disabled for new-user mode (correct) but has no tooltip explaining why.
**Fix:** Radix Tabs handles focus by default; verify and add a `title` / `aria-describedby` on the disabled trigger with explanation. Add a small "•" dot on the trigger label when overrides are dirty (depends on C2 fix).
`user-form.tsx:362-387` — opens on submit. Radix returns focus to the submit button after close (good), but the dialog's `<AlertDialogAction>` triggers `persist()` without disabling itself during the network call; rapid double-click can fire two PATCHes. Also `disabled={loading}` is set on action but not on `<AlertDialogCancel>` re-enable timing.
**Fix:** add a `submitting` guard or rely on existing `loading` state for both buttons; close dialog only after `persist()` resolves.
Across `user-list.tsx`, `user-card.tsx`, `documents-hub.tsx`, `berth-status-suggestion-dialog.tsx`, status pills with `<ShieldCheck>`, `<Power>`, `<PowerOff>`, `<Globe>`, etc., the icons supplement text — they should carry `aria-hidden="true"` so screen readers don't double-announce. Mixed across the codebase; some lucide imports get it, most don't.
`src/components/clients/client-interests-tab.tsx:217` uses Vaul `<Drawer>` for an interest preview, while every other detail-preview surface (yacht preview, company preview, reservation preview) uses `<Sheet>`. Vaul drawers are intended for mobile bottom-sheets; using it for an inline preview on desktop is inconsistent.
**Fix:** standardize on `<Sheet side="right">` for desktop right-rail previews; reserve `<Drawer>` for the mobile More menu (`more-sheet.tsx`).
`user-list.tsx:135,147,180` — relies on native browser tooltips. They don't appear on touch and don't surface to screen readers; the `<span className="sr-only">` carries the label which is correct, but consider Radix `Tooltip` for parity with the rest of the app.
`src/app/(auth)/login/page.tsx:106,123` — `#007bff` / `#0069d9` hex hardcoded instead of using `brand-500` / `brand-600` design tokens. Same issue in sidebar.tsx:190,196,379 (`#3a7bc8`).
### L4 — `formatAction` duplicated locally in matrix instead of in `constants.ts`
`user-permission-matrix.tsx:100-102` re-implements the title-case replace. Move to `constants.ts` as `formatPermissionAction` (used in 3+ files: role-list.tsx, role-form.tsx, matrix).
Across `bulk-archive-wizard.tsx`, `hard-delete-dialog.tsx`, `smart-archive-dialog.tsx`, `smart-restore-dialog.tsx`, `pdf-reconcile-dialog.tsx`, `user-settings.tsx:321`, etc. Need a shared `<Callout tone="warning|info|danger|success">` primitive that reads from design tokens.
- Form helper coverage: `react-hook-form + zodResolver`, `PhoneInput`, `CountryCombobox`, `TimezoneCombobox`, `InlineEditableField`, `InlineTagEditor` are present and used consistently in client/yacht/company/interest forms.
-`parseBody` + `errorResponse` envelope convention holding for new endpoints checked.
-`ConfirmationDialog` correctly returns focus and traps focus via Radix `AlertDialog`.
-`StatusPill` is the right primitive; just under-adopted (M1, M2).
- Mobile bottom tabs handle `aria-current` correctly (template for H6).
- UserCard already adds `aria-label="Actions for ${displayName}"` on the icon-only `MoreHorizontal` trigger.
---
## 3. Data model + migrations + relations audit (data-model-auditor)
# Data Model + Migrations + Relations Audit
Scope: `src/lib/db/schema/*.ts` (24 files) and migrations 0000–0055.
~92 tables, multi-tenant on `port_id`. Drizzle ORM + `postgres-js`.
---
## CRITICAL
### C1 — No prod migration runner; 0052 uses `CREATE INDEX CONCURRENTLY`
`berth_pdf_versions.parseResults`. The `permission-overrides` PUT route is well
sanitized (`ALLOWED_RESOURCE_ACTIONS` allow-list before write). `userProfiles.preferences` is validated and 8KB-capped at the API. The others rely on per-caller validators only.
### M8 — `scratchpadNotes.linkedClientId` crosses ports without enforcement
`db.update(user).set({ email: ... })` writes the new email directly to the Better Auth `user` table. The Better Auth `account` table (`src/lib/db/schema/users.ts:194-210`, `providerId='credential'`) carries an `accountId` column that is typically the user's email — used by Better Auth's password-login flow to resolve a credential row. The update does NOT touch `account.accountId`, does NOT invalidate active sessions, does NOT update `account.updatedAt`, and does NOT use Better Auth's admin API (`auth.api.updateUser` / `setEmail`). Failure modes:
- The whole flow runs **outside any transaction** — `userProfiles` update (line 230), `user` update (line 247), `userPortRoles` update (line 281), audit log, and notification-fire are five independent writes. A failure between them leaves partial state with no rollback.
- **No idempotency under retry**: there is no guard that the email actually differs from the current `account.accountId`, and the email-change notification is fire-and-forget — a retried admin request re-fires the courtesy email and rewrites all rows.
Fix: route through `auth.api.updateUser` (or write `account.accountId` + bump session invalidation) and wrap in a transaction.
### C2. `handleDocumentCompleted` orphan blobs on mid-flight failure
5.`db.update(documents).set({status:'completed', signedFileId})` (line 1185) — **if this fails** (e.g. transient connection loss after `files` insert), the document keeps `signedFileId = NULL`.
On the retry from Documenso, the early-return short-circuit is bypassed (signedFileId still NULL). The function re-downloads, **re-generates a new UUID** (`crypto.randomUUID()` at line 1131), re-puts to a new key, inserts a second `files` row, and only then updates the document. The first blob from step 2 + the first `files` row are now orphaned (unreachable via document, but the file row still exists and may surface in aggregated listings with no docs link).
Additionally, the `catch` block (line 1244) marks `status='completed'` with no `signedFileId` — this means the document is presented to the rep as "complete" while the signed PDF was never persisted. Subsequent webhook retries will retry (no early-return) but if Documenso stops retrying after Nth attempt, the document is permanently stuck "completed with no file."
Fix options: (a) wrap `files.insert` + `documents.update` in one transaction; (b) delete the blob in the catch when the file row insert succeeds but the document update fails; (c) refuse to mark `status='completed'` in the catch — leave as-is so the next retry / cron poll succeeds.
---
## HIGH
### H1. Notification-email worker HTML injection via `notif.link`
`notif.description`, `notif.title`, and `notif.link` are interpolated into HTML with no escaping. `notif.link` is mostly internal-generated (`/documents/{id}`) but several call sites push user-derived values into `description` (filenames, client names, custom alert text). A `description` of `<img src=x onerror=...>` ships as live HTML to the recipient's inbox. Lower-severity than C1 because most notifications are admin-only and the recipient is internal staff, but still an XSS-via-email primitive. Use the same `renderEmailBody` (allowlist) helper the send-out flow uses.
`scanForDuplicates` returns candidates, then `markBestDuplicate` writes `duplicateOf`. Two concurrent dedup-engine runs on a pair `(A,B)` can each mark the other as the duplicate → mutual `duplicateOf` cycle, both archived later by `mergeDuplicate`. No advisory lock, no transaction encompassing scan + update. Also: `scanForDuplicates` does not filter `archived_at IS NULL`, so already-merged sources can resurface as candidates.
`tableForEntity` is defined and immediately `void`-discarded — every CRUD branch inlines its own switch. New entity types (e.g. `residential_clients`) added to the type union are silently missed by inlined branches because the exhaustive-switch compiler check is absent. This is the actual drift-vector for the polymorphic dispatch CLAUDE.md called out. Either delete the helper or refactor every CRUD operation to go through it.
if (userSockets.length >= 10) return next(new Error('Maximum connections reached'));
```
Between `fetchSockets()` and the eventual `socket.join(`user:${userId}`)` at line 132, another concurrent handshake can pass the same check. Under burst reconnect (e.g. flaky network across many tabs), users get 11+ sockets. The Redis adapter's `fetchSockets` is multi-pod-aware, but the gating is not atomic. Use a Redis `INCR` keyed by `user:${id}:conn_count` with TTL fallback, decrement on disconnect.
`verifyDocumensoSecret` short-circuits on `length !== expected.length` before `timingSafeEqual`. Combined with the linear scan across all per-port secrets, response-time deltas leak the number of ports and the length of each secret. Marginal but easy fix: pad to fixed size.
### H6. Global Documenso secret silently drops events under multi-tenant ambiguity
`resolveWebhookDocument` correctly refuses to mutate when documensoId matches multiple ports AND no portId was passed. The webhook route now resolves portId from the matched secret (good — see comment at line 138-143). But the global `env.DOCUMENSO_WEBHOOK_SECRET` fallback entry returns `portId: null` (`port-config.ts:370`), and any port still using the global secret falls back to the "ambiguous → refuse" path. **Result:** if two ports share the global secret, valid completion events get silently dropped instead of routed. The dedupe + dead-letter on the inbound side doesn't surface this — it just looks like Documenso never delivered. Recommend: require per-port secrets for production and warn loudly when more than one port resolves to `portId: null`.
---
## MEDIUM
### M1. Storage migration loads each blob fully into Node memory
`for await (chunk of stream) { chunks.push(chunk) }` materializes the full blob in memory twice (source read + verify re-read) per file. A 200MB signed PDF or GDPR export blows the worker. Consider piping through `crypto.createHash('sha256')` + tee to the target backend instead of `Buffer.concat`. The pre-flight free-disk check (line 298-310) does `Promise.all(refs.map(head))` for every blob in the table — for large `files` tables that's thousands of round-trips before any copy starts.
The IIFE that builds next-in-line notifications fires after `softDelete(interests, ...)` and `evaluateRule` — both already queued via `void`. If the IIFE throws after the interest is archived but before notifications send, only a `logger.error` lands; the archived interest stays archived with no rep notification. Acceptable as best-effort, but the dossier doesn't run inside the same audit-context request (the `createAuditLog` call happens earlier), so an operator reading the audit trail sees "archived" without seeing what notifications were attempted. Consider attaching the dossier result to the audit metadata.
### M3. `attachWorkerAudit` always records `portId: null`
Every job-failure audit row is written with `portId: null`. Multi-port operators querying their port-scoped audit log will not see worker failures that affected their port (e.g. a `documenso-void` job carrying portId in `job.data`). The worker has access to `job.data.portId` for most queues — extract it where present.
Hardcoded `Set` requires manual sync against `scheduler.ts`. Typos silently demote cron heartbeats to regular completion logs. Either co-locate or compute from the scheduler module at boot.
`INFLIGHT_STATUSES = ['draft', 'sent', 'partially_signed']` includes `draft`. CLAUDE.md describes the UI section as "Signing-in-progress" — drafts have not been sent. Confirm intent.
### M6. Documenso secrets stored plaintext in `system_settings`
`listDocumensoWebhookSecrets` reads `systemSettings.value` directly — no decryption. SMTP/IMAP passwords are AES-256-GCM-encrypted per CLAUDE.md; the Documenso webhook secret should be too.
`process()` body is `// TODO(L2)`. Any job pushed to the `import` queue silently completes with no work — every CSV import is a silent success if the producer side ships first.
- **`handleDocumentCompleted` idempotency gate** (line 1110) is correct _when reached_. The hazard is the partial-write window above (C2), not the gate itself.
- **`resolveWebhookDocument`** correctly refuses to mutate on multi-port ambiguity.
- **Socket auth middleware** (`server.ts:91-124`) cross-checks the client-supplied `auth.portId` against `userPortRoles` — closes the prior tenant-room hijack.
- **Storage filesystem backend** correctly refuses to start when `MULTI_NODE_DEPLOYMENT=true` (`filesystem.ts:218`) using the zod-validated env, not raw `process.env`.
- **Magic-byte verification** is enforced both for brochures (`brochures.service.ts:241-263`) and berth PDFs (`berth-pdf.service.ts:234-262`) with delete-on-mismatch cleanup.
- **File-aggregation projection** (`files.ts:316-379, 526-579`) applies `port_id` at the entry-point assert, on `companies.port_id` / `clients.port_id` / `yachts.port_id` joins, on `files.port_id` in the predicate, and on the `documents` LEFT JOIN's residual (line 567). Defense-in-depth is consistent.
- **Webhook worker** has DNS-rebinding SSRF re-resolution at dispatch (`webhooks.ts:18-45`) and dead-letter handling with operator notifications.
Also dead — these `:portSlug/settings/<X>` paths have **no folder** under `src/app/(dashboard)/[portSlug]/settings/`; the only subroute that exists is `profile/`:
-`/:portSlug/settings/email`
-`/:portSlug/settings/branding`
-`/:portSlug/settings/templates`
-`/:portSlug/settings/storage`
-`/:portSlug/settings/recommender`
-`/:portSlug/settings/tags`
-`/:portSlug/settings/notifications`
These look like aliases that were intended to deep-link inside `/settings` tabs but never wired up. Either redirect them to `/admin/<x>` (which all exist) or render real `settings/<x>` pages.
### C2. Webhooks bypass the platform-error pipeline
`src/app/api/webhooks/documenso/route.ts` is the only webhook route in the repo and it does **NOT** call `errorResponse(...)` / `captureErrorEvent(...)`. The handler always returns 200 with `logger.error(...)` only, so admin/errors never sees Documenso webhook crashes — the CLAUDE.md/docs imply errors flow into `error_events` universally but webhooks are silently outside that flow. Recommended: wrap the handler in a try/catch that calls `captureErrorEvent({ statusCode: 500, error, metadata: { source: 'webhook', event } })` before returning 200.
---
## HIGH
### H1. PDF templates hard-code `en-GB` date locale (ignores user prefs)
CLAUDE.md and the new dashboard greeting / timezone-drift banner suggest the rep's locale + timezone is honoured end-to-end. It isn't — at the PDF / signing-email surface we silently revert to `en-GB`. User-preference `timezone`/`locale` from `user_profiles` is plumbed nowhere into these templates.
### H2. PDF templates hard-code `USD` price formatting & build raw `Number().toLocaleString()` strings
-`interest-summary-template.ts:112`, `berth-spec-template.ts:127,172` — `berth.priceCurrency ?? 'USD'` followed by `Number(price).toLocaleString()` (no Intl currency formatter, no grouping conventions per locale).
-`reports/pipeline-report.ts:93`, `reports/revenue-report.ts:78,86` — `Number(...).toLocaleString()` with no currency code at all in revenue report.
Single-source `formatCurrency()` exists at `src/lib/utils/currency.ts` and is used everywhere else — these templates should call it.
-`src/components/dashboard/revenue-forecast.tsx:25` — same
-`src/components/dashboard/pipeline-value-tile.tsx:45,47` — same (the inner data field is `pipelineValueUsd` — backend converts to USD before sending). The "pipeline value tile" claim that the comment says "USD-denominated" is fine, but the KPI / forecast tiles silently render Euro/GBP ports as USD.
The "Conventions / Auth" section currently implies `user_port_roles.role` is the leaf authority. New developers won't know to apply user-level overrides when reasoning about effective permissions.
### H5. `feedback_pwa_assets_pending` memory is stale
User memory says PWA assets (`icon-192.png`, `icon-512.png`, `icon-512-maskable.png`) must be added before shipping Phase B scanner. **All three exist in `public/`** plus `apple-touch-icon.png`. Memory should be cleared.
---
## MEDIUM
### M1. `archiveBrochure` has no `createAuditLog` call
`src/lib/services/brochures.service.ts:191` — service-level archive (`archivedAt` + `isDefault: false`) commits without an audit row. Every other archive/delete in this branch (yachts, clients, companies, interests, berths, documents, document-folders, files, invoices, document-templates, email-accounts, users, roles, portal-auth, custom-fields) creates audit logs. Brochures is the outlier — same UX risk as the others (admin can swap default brochure with no trail).
### M2. PII risk: portal-auth logs the email address of unknown / disabled-portal users
`src/lib/services/portal-auth.service.ts:356,373,423` log `email`, `user.email`. Logger redact paths cover passwords / tokens / encrypted blobs but **not**`email` / `*.email`. For most CRM logging this is fine (emails are not secret in this app), but the portal-reset paths specifically log emails of users **outside** the active session — a quiet PII surface in log aggregators. Recommend either (a) hash-prefix the email (`hash6(email)`) before logging, or (b) accept-but-document.
-`src/app/api/webhooks/documenso/route.ts:122,156,187,243,258,262` — six `logger.info(…)` per webhook fire (duplicate skip, lifecycle event, unhandled event type). At realistic Documenso traffic + retry pressure this is noisy. Consider downgrading the `'Documenso lifecycle event'` line at L258 (fires on **every** valid event) to `logger.debug`.
-`src/lib/services/email-threads.service.ts:290,298,358` — IMAP sync logs `mailbox.exists`, `messageCount`, `'No new messages to sync'` at info on **every** poll. At 5-min poll cadence × 24h × N accounts this floods info-level logs. Should be `logger.debug`.
### M4. Timezone-aware reminder `dueAt` storage looks correct but UI hands off naïve strings
`reminders.dueAt` is stored as `TIMESTAMPTZ` (`reminders.service.ts:179` — `new Date(data.dueAt)`). The validator accepts an ISO string. `<DateTimePicker>` in date-time inputs reads `new Date(input)` from the browser — interpretation is **local-TZ** for `YYYY-MM-DDTHH:mm` strings, **UTC** for full ISO with `Z`. Worth a focused look on the picker component to confirm it emits `Z`-terminated ISO (else "reminder at 9 AM" means 9 AM browser-local on creation, but server's `formatInTimeZone` against the rep's chosen TZ will misalign). I did **not** open `<DateTimePicker>` itself in this audit — flagging as a 30-minute follow-up.
### M5. CLAUDE.md numbered-spec section frames `01-15` as authoritative
> "Numbered spec files in repo root (01-…through 15-…) contain detailed architecture decisions, feature specs, DB schema docs, API catalog, and implementation sequence."
These specs document the **pre-rebuild Nuxt 3 / NocoDB system being migrated FROM**. `01-CONSOLIDATED-SYSTEM-SPEC.md` header reads "Compiled: 2026-03-11" and the stack tables describe Nuxt 3 SPA, NocoDB, Keycloak OIDC, etc. — none of which are the live Next.js + Drizzle + better-auth stack. New contributors reading CLAUDE.md will be sent down the wrong path. Recommend reframing to "legacy reference for the rebuild target" or moving them to `docs/legacy/`.
### M6. BACKLOG.md doc-folders entry stale vs reality
`docs/BACKLOG.md` E. "Hidden / stubbed UI tabs" still lists Company Documents tab as ✅ landed 2026-05-08 but in the same section says **Berth Waiting List + Maintenance Log tabs** are "Removed entirely; revisit if/when product asks" — yet `src/components/berths/berth-tabs.tsx` still imports and renders the tabs strip (the comment in CLAUDE.md is silent on this). Not blocker — just a doc/code drift.
### M7. Admin sections browser missing two real admin routes
`src/components/admin/admin-sections-browser.tsx` registers 30 hrefs. Two `/[portSlug]/admin/*` routes exist but are **NOT** surfaced in the browser:
-`/admin/brochures` (full UI exists at `src/app/(dashboard)/[portSlug]/admin/brochures/page.tsx`)
-`/admin/errors` (super-admin platform-errors inspector, real route)
The NAV_CATALOG catalog (Cmd-K) covers "Platform errors" via the (dead — see C1) `error-events` href but no entry for `/admin/brochures`. Reps cannot discover the brochure admin surface from either the section card grid or global search.
admin-sections-browser keywords list (settings card) is in sync with `settings-manager.tsx` KNOWN_SETTINGS keys today (21 keys, 39 aliases). **However**, two settings exist in production that are NOT in either list:
-`documenso_signing_order` (CLAUDE.md L46) — typeable via Documenso admin page, not the generic Settings card; reasonable to omit but flag if you want unified search.
-`documenso_redirect_url` — same.
Not a bug — just confirming the drift surface is intentional.
---
## What was NOT in scope but worth a quick note
- **Audit-coverage spot-check** for sensitive mutations: clients/yachts/companies/interests/berths/documents/document-folders/document-templates/invoices/users/roles/portal-auth/files/custom-fields/document-sends/email-accounts ALL call `createAuditLog`. Only `brochures.archiveBrochure` is missing (M1). No other gaps spotted in services that I sampled.
- **Pino redact paths** cover passwords, tokens, secrets, encrypted blobs, Authorization headers, cookie headers, two-level nesting — comprehensive. Only soft gap is `email` field (M2).
- **Error pipeline**: `errors.ts → captureErrorEvent` is invoked on every 5xx route response; the `error_events` table is read by admin/errors. Looks complete for API routes — gap is webhooks (C2).
- **Country/nationality**: consistently stored as ISO-3166-1 alpha-2 across clients, companies, residential — validators centralized in `src/lib/validators/i18n.ts`. Good.
`src/lib/services/gdpr-bundle-builder.ts` enumerates only a subset of tables that hold the data subject's PII. The following tables reference the client (`client_id` FK) but are **NOT** included in the bundle:
-`portal_users` — the portal account itself (email, name, `lastLoginAt`, `isActive`, `createdBy`). Strictly required: a copy of the account record is core "data we hold about you."
-`email_threads` / `email_messages` — full inbound/outbound correspondence including `bodyText`, `bodyHtml`, attachment IDs. This is the most PII-dense table in the system.
-`document_sends` — brochure / send-out audit with `recipient_email` (`brochures.ts`).
-`reminders` — operations table with `clientId` FK.
-`formSubmissions` (public form intake) — already collected via `documents` for the linked path, but rows where `client_id` is set directly are missed.
-`files` — files attached directly to a client (`files.client_id` ≠ via `documents`). The builder pulls `documents` only.
-`scratchpadNotes.linkedClientId` — rep-side free-text notes that reference the client.
-`clientMergeLog` — historical merge records that survived earlier deduplications.
-`contact_log` (referenced from `operations.ts`).
-`website_submissions` (raw inbound inquiries before they were promoted to interests).
The bundle currently advertises itself as the Article-15 dump. Hand-delivering it would expose the controller to a regulator finding of incomplete disclosure. **Fix:** widen `buildClientBundle` to cover every `client_id`-referencing table (the schema grep below produces the complete list); the audit-log limit of 500 events should also be lifted or paginated for long-tenured clients.
### C2. "Right to be forgotten" leaves email correspondence bodies intact
`client-hard-delete.service.ts` nullifies `emailThreads.clientId` so the thread (with `bodyHtml` / `bodyText` of every inbound + outbound message + the subject's address in `from_address` / `to_addresses`) survives the delete in perpetuity. Same pattern for `files.clientId`, `documents.clientId`, `formSubmissions.clientId`, `reminders.clientId`, `documentSends.clientId`. The justification in the file ("keep their audit history") is reasonable for _audit metadata_ but the actual PII content (email body, file contents, form answers, recipient emails on brochure sends) is preserved verbatim. A subject who exercised their right to erasure has not, in practice, been erased.
`src/app/api/auth/resolve-identifier/route.ts` returns the canonical email when a known username is supplied (line 88: `return NextResponse.json({ email: rows[0]!.email })`). The miss-path returns a synthetic `.invalid` address, which protects hit/miss equality, but **any successful hit leaks the linked email** to an anonymous caller. Rate-limit is 5/15min/IP — sufficient to thwart wordlist brute-force but trivial to walk known/leaked usernames. This is also the entire point a malicious actor would call this endpoint (compromised-credentials stuffing).
**Fix:** don't echo the resolved email back to the client. Instead, set a short-lived signed cookie / Redis key keyed by the IP+identifier that the subsequent `signIn` call consumes, or hand the resolved email straight to Better Auth server-side and return only `{ ok: true }`.
### C4. Audit-log metadata is unmasked and stores raw PII forever
`src/lib/audit.ts``maskSensitiveFields` covers `oldValue`/`newValue` only — `metadata` is written raw. Multiple call-sites stuff full email addresses into metadata:
Compounded with **C5** (no audit-log retention), every staff member's, invitee's, and portal user's email lives in `audit_logs.metadata` indefinitely with `ip_address` + `user_agent` next to it. This is GDPR data-minimisation/storage-limitation breach territory.
**Fix:** extend `maskSensitiveFields` to walk `metadata` recursively, or stop emitting full emails into metadata (use user IDs + a join on demand). The masking set also needs `emailAddress` and `sentTo` aliases.
`src/lib/queue/scheduler.ts` registers retention crons for `ai_usage`, `error_events`, `website_submissions`, but **not**`audit_logs`. The schema docs the table being kept indefinitely (no pruning worker exists). With IP + user-agent on every row, plus PII in metadata (C4), the table grows unbounded and replays PII forever.
**Fix:** add a `audit-log-retention` maintenance job. Recommended split: keep `severity ∈ {warning, error, critical}` and `source = auth` for 2 years (legal/security), prune everything else after 12 months. Make the window admin-configurable.
### H2. `error_events` request-body excerpts redact only secret-shaped keys
`error-events.service.ts``SENSITIVE_KEYS` redacts `password`/`token`/`apiKey`/`creditCard`/`ssn` etc. — but NOT `email`, `phone`, `name`, `dob`, `address`. Any 5xx on `POST /api/v1/clients`, `POST /api/v1/portal-users`, `POST /api/v1/clients/[id]/contacts`, `POST /api/v1/admin/users` lands the requester's full client-create payload in `error_events.request_body_excerpt`. Retention is 90 days (good) but the captured rows are visible to every super-admin via the inspector. **Fix:** add PII keys to `SENSITIVE_KEYS` or whitelist-only the body schema per route.
### H3. Email recipient address logged at `debug` level
`src/lib/email/index.ts:154` — every outbound email logs `{ to, originalTo, subject }`. In prod this is `info` only if `LOG_LEVEL=debug`, so usually safe, but the `originalTo` field also leaks the redirect-target's real address when `EMAIL_REDIRECT_TO` is set in dev. Tighten to `messageId + portId + bool` once redirect path is exercised.
### H4. Portal `lastLoginAt` & email kept after client hard-delete
On client hard-delete the `portal_users.client_id` cascade fires, so the portal user is removed — good. But `portal_users.email` has a global unique index (`idx_portal_users_email_unique`) with no `port_id`. A previously-deleted portal user blocks a new portal account at a different port from re-using that email until/unless the cascade fires. More importantly, if cascade ever doesn't run (e.g. archive-only, no hard delete), the portal account row survives with the email. Verify the archive path also disables/erases the portal user, or document the asymmetry.
### H5. Encryption-key rotation is non-incremental for SMTP/IMAP creds
`src/lib/utils/encryption.ts` hard-codes a single env var (`EMAIL_CREDENTIAL_KEY`) with no key-version or KID stored on the ciphertext. Rotating the key requires an offline mass re-encrypt; there is no migration path. The same applies to the S3 secret key (`storage/s3.ts:74`), webhook secret (`workers/webhooks.ts:116`), and storage_proxy_hmac (`storage/filesystem.ts:415`). Each decrypt-failure path falls through silently. **Fix:** prefix ciphertexts with a `kid` field, support 2 active keys at once, and ship a rotation script that re-wraps ciphertexts to the new key.
### H6. Activation/reset tokens travel in URL query strings
`portal-auth.service.ts:147 / 408` and `crm-invite.service.ts:71 / 233` ship `?token=…` in the activation/reset links. The token hash is stored server-side (good), but URL-borne tokens land in browser history, reverse-proxy access logs, Cloudflare logs, and `Referer` headers if the activation page links anywhere external (e.g. terms-of-service). Common pattern but worth flagging — consider hash fragments (`#token=…`) which browsers never put in `Referer`.
### H7. IP address recorded on every audit event without a lawful-basis note
`audit_logs.ip_address` (system.ts:38) and the legacy second copy (line 305) are populated unconditionally. Storing IPs is lawful under "legitimate interest" for security-relevant events, but for routine `update`/`view`/`create` of a client record the lawful-basis argument is much thinner under recent EU regulator guidance. **Fix:** only retain `ip_address` on `source ∈ {auth, webhook}` and on `severity ∈ {warning, error, critical}`; null it on routine `user`-source events at write time.
`search.service.ts:2147` saves the raw search term to Redis under `recent-search:<userId>:<portId>`. If a rep types a client's email/phone/SSN to find them, that string lives in Redis with a 7-day TTL (per the constant) and is not in the GDPR bundle. Low-volume, but document and add to the bundle.
### M2. GDPR-export confirmation email contains client name verbatim
`client-hard-delete.service.ts` sends a confirmation code email to the requester with the deleted client's `fullName` in subject + body. Reasonable for human verification, but it means the operator's mailbox (often Gmail/Outlook) holds the to-be-erased client's name after deletion. Document this in the privacy notice or strip to initials.
### M3. GDPR export ZIP retention overlaps with subject-erasure
The bundle expires 30 days after generation (`EXPIRY_DAYS = 30`) — but if a subject requests _both_ export and erasure inside that window, the staged ZIP in MinIO will outlive the database row. The cleanup cron only checks `expires_at`. **Fix:** when `hardDeleteClient` runs, delete any non-expired `gdpr_exports` blobs for that client immediately.
`docs.documenso webhook handler` logs `signatureHash` only (good), but `document_sends` rows store full `recipient_email` on `set null` cascade — so when the linked client is hard-deleted, the recipient email survives on the send row. Same pattern as C2 but for the brochure channel.
### M5. `portal_auth_tokens.tokenHash` is SHA-256, not constant-time-compared
Tokens are hashed before storage (good) but the lookup `where idx_portal_tokens_hash_unique` uses normal equality. Since the index lookup is `O(1)` indexed-equality, timing attacks are not viable here — flagged for documentation only.
### M6. `error_events.error_stack` may contain user-supplied strings
Stack traces are 4 KB capped and only fire on 5xx, but PG-driver errors include the offending statement / parameter values in `.message` (e.g. duplicate-key violations expose the conflicting email). Already mitigated by error-coding most known cases through `CodedError`, but a defensive scrub on `errorMessage` for `@`-shaped or `+\d{6,}` substrings would harden the inspector.
### M7. `EMAIL_REDIRECT_TO` is enforced only by env, not by a build assertion
The README warns it must be unset in production but there's no runtime guard. A misconfigured prod could silently redirect ALL outbound client mail to a single inbox. **Fix:** in `src/lib/env.ts`, refuse `EMAIL_REDIRECT_TO` when `NODE_ENV === 'production'`.
### M8. Cross-port `portal_users.email` unique index leaks tenancy
Multi-tenant model says ports are isolated, but the global-unique email index means port A can probe whether email X is already a portal user at any other port by attempting to invite them and reading the conflict error. Tiny enumeration vector, fix by scoping the unique index to `(port_id, lower(email))`.
---
## Notes / good practices observed
-`audit.ts``maskSensitiveFields` exists and is applied to `old/new` JSON.
-`logger.ts` ships a thorough `redact.paths` list covering auth headers, encrypted credentials, cookies.
`src/lib/env.ts:41` declares it `z.string().email().optional()` with no `NODE_ENV` constraint. `src/lib/email/index.ts:131-133` silently rewrites every recipient when it is set. CLAUDE.md says "**must be unset in production**" but nothing enforces it: a stray prod `.env` value would funnel every client/portal/EOI invitation to one address with zero alarms. The only signal is a `logger.debug` line (`index.ts:154-156`) — debug, not warn, and no startup banner. The companion `documenso-client.ts`, `email-compose.service.ts`, and `webhooks.ts` paths also silently honour it. Add a refinement in `env.ts` rejecting it (or at least `logger.fatal`-ing) when `NODE_ENV === 'production'`, and emit a startup `logger.warn` whenever it is set so it shows up in container logs.
### C2. Unescaped URL interpolation into `href` attributes (XSS-able in browser previews)
Every template inlines `${data.link}` / `${data.signingUrl}` / `${data.loginUrl}` / `${item.link}` / `${data.inboxLink}` / `${data.crmDeepLink}` / `${data.signingUrl}` / `${crmUrl}` (the last is escaped in `inquiry-sales-notification.ts:34` — sole exception) directly into `href="…"` and into the visible link text. `escapeHtml` is applied to every recipient name and copy string but **never to URLs**:
Most URLs come from server-built strings (token endpoint + base URL), but `notification-digest.items[].link` is sourced from notification rows whose deep links can include user-typed entity titles / search queries depending on the producer. A single `"` in any of those will break out of the attribute. Email clients (Apple Mail, Outlook Web) render the resulting HTML and attribute injection becomes click-jacking / open-redirect. Cheapest fix: pass every URL through `encodeURI` or an `escapeAttribute` helper before interpolation, and reject `javascript:` / `data:` schemes at the helper level. None of the templates currently verify `https://` prefix.
---
## HIGH
### H1. Template-subject override mechanism is silently disconnected for ~half the catalog
`src/lib/email/template-catalog.ts:39-98` advertises 8 templates as customisable, and `src/components/admin/email-templates-admin.tsx` exposes a subject editor. But several templates **don't accept `overrides.subject`** and so the admin's edit is silently ignored:
-`inquiry-client-confirmation.ts:23-26` — no `overrides.subject` path
-`inquiry-sales-notification.ts:24` — same
-`residential-inquiry.ts:19, 61` — both functions, no override path
-`crm-invite.ts:26` — no override path
Only `portal-auth.ts` (activation/reset) and `document-signing.ts` honour `overrides.subject`. Admins editing the "Inquiry — client confirmation" subject in `/admin/email-templates` see the green "Saved" toast and nothing changes. This is the kind of bug users don't report; they assume their override worked.
### H2. Catalog `defaultSubject` strings DO NOT MATCH the literal subjects the code emits
The admin UI shows `Default: <catalog string>` so users can tell whether they have customised, but every comparison is broken because the strings diverge from the actual templates:
| `crm_invite` | `You have been invited to {{portName}} CRM` | `You're invited to the ${portName} CRM` |
| `inquiry_client_confirmation` | `We received your inquiry — {{portName}}` | `Thank You for Your Interest in Berth ${mooringNumber}` (or `…in a ${portName} Berth`) |
| `residential_inquiry_client_confirmation` | `We received your residential inquiry — {{portName}}` | `Thank You for Your Interest - ${portName} Residences` |
Combined with H1 this means the entire admin-customisation surface is half wired. Pick one of: (a) wire `overrides.subject` through every template and remove the divergence, or (b) drop the catalog rows for templates that can't be customised yet.
### H3. Email Settings page exposes dead form fields
`src/app/(dashboard)/[portSlug]/admin/email/page.tsx:34-48` lets admins set `email_signature_html` and `email_footer_html`. `getPortEmailConfig` (`port-config.ts:153-154,179-180`) reads them into `PortEmailConfig.signatureHtml` / `footerHtml`. But **`sendEmail` (`src/lib/email/index.ts`) never reads or injects them**, and `shell.ts` only reads the unrelated `branding_email_footer_html` / `branding_email_header_html` keys (via `getBrandingShell` → `getPortBrandingConfig`, lines 39-40 of shell.ts).
Result: the "Default signature (HTML)" and "Email footer (HTML)" controls on `/admin/email` are write-only sinks. Admins customise the footer; outbound emails never include it. There is a real customer-confidence hit here — a port admin will set a legal disclaimer expecting it on every send. Either (a) wire `cfg.footerHtml`/`signatureHtml` into `renderShell` or `sendEmail`, or (b) delete the fields from the admin page and consolidate on the Branding-page keys.
### H4. `residential-inquiry.ts` returns NO plaintext fallback
`residentialClientConfirmation` (line 41) and `residentialSalesAlert` (line 78) return `{ subject, html }` only — no `text`. Every other template returns `text`. Lack of a `text` part materially hurts spam scoring and breaks plain-text-only readers (some BlackBerry, screen-reader bridges, legacy MTAs). `sendEmail` (`index.ts:144-152`) honours `text` when present, so the consequence is "no plain-text MIME part is attached" — Gmail will still render it but Spamassassin's `MIME_HTML_ONLY` adds points.
### H5. `inquiry-sales-notification` includes `crmUrl` (which the admin may set) without scheme validation
`inquiry-sales-notification.ts:34` does `escapeHtml(crmUrl)` then drops it into both `href` and visible text. Escaping prevents attribute breakout but a `javascript:` or `data:text/html` scheme survives entity-encoding. Validate the scheme is `https?:` server-side before passing in (the producer probably already does; defence-in-depth here is one regex).
---
## MEDIUM
### M1. Admin-authored `emailHeaderHtml` / `emailFooterHtml` injected raw
`shell.ts:39-40, 64-66` interpolates branding HTML directly: `${headerHtml ? \`<div>${headerHtml}</div>\` : ''}`. Source is `system_settings.branding_email_header_html` (admin-only write). An admin account compromise → arbitrary HTML in every outbound email (`<a href="javascript:…">`, tracking pixels exfiltrating recipient IPs, phishing forms in some clients). Email clients largely strip script, but `<form action=…>`, `<meta http-equiv="refresh">`, and CSS-position overlays still work in Apple Mail / Outlook desktop. Mitigation: run admin input through a server-side sanitiser (DOMPurify / sanitize-html with an email-safe allowlist).
`shell.ts:42-74` ships no `<meta name="color-scheme" content="light dark">` / `<meta name="supported-color-schemes">` and no `@media (prefers-color-scheme: dark)` rules. Apple Mail and Gmail auto-invert backgrounds: the `#ffffff` card stays white but body text (`#333333`) gets pseudo-darkened in some clients, and the `#666` muted copy can drop below contrast threshold. Cards with `box-shadow: 0 2px 4px rgba(0,0,0,0.1)` render as halos in dark mode.
The shell has no `<!--[if mso]>` conditionals. The CTA buttons are CSS-padded `<a>` — Outlook 2016/2019 on Windows renders them as tiny underlined text (the link works but the button shape is gone). Recommended: VML rect fallback inside `<!--[if mso]>` for every CTA, or switch to bulletproof-button pattern (`mso-padding-alt`, `text-decoration:none` etc.).
### M4. Background image won't render where it matters most
`shell.ts:55` sets `background-image: url('…Overhead_1_blur.png')` on the outer `<table>`. Outlook strips background-image entirely (no VML fallback supplied); Gmail mobile sometimes too. The `background-color:#f2f2f2` fallback works but the brand impression is lost in the highest-volume client. Either drop the bg image (CLAUDE.md flags moving the asset off `s3.portnimara.com` anyway) or add VML rect for Outlook.
No hidden inbox-preview text. Currently the first visible line ("Welcome to {portName} CRM" or "Just a quick reminder") leaks into the preview pane after the subject. A 1px hidden preheader (`<div style="display:none;max-height:0;overflow:hidden;">…</div>`) is one of the highest-ROI deliverability tweaks; missing here.
### M6. Logo `width="100"` without 2x source / explicit height
`shell.ts:62`. Apple Mail on retina renders this scaled — acceptable since the source PNG is 250px wide. But `height` is unset which forces some clients to recompute mid-render (jank). Add `height="100"` (assuming square) and the asset is fine.
---
## LOW
- **L1. Hardcoded `en-GB` date locale.** `document-signing.ts:141` calls `toLocaleString('en-GB', …)`. CRM is positioned as multi-port; once a non-UK port arrives this is wrong. Read locale from port-config.
- **L2. No `List-Unsubscribe` / `List-Unsubscribe-Post` headers.** `sendEmail` adds none. Gmail's Feb-2024 bulk-sender requirements made `List-Unsubscribe: <mailto:>` table-stakes for any sender exceeding 5k/day. Transactional senders technically exempt, but with notification digests + inquiry confirmations flowing through the same SMTP path, hitting that threshold is plausible. One-line fix.
- **L3. No `Message-ID` / `In-Reply-To` threading for digest-style mails.** Each digest is a new thread; users will hate this once volume rises.
- **L4. `logger.debug` in `sendEmail` (`index.ts:154`) emits recipient address.** PII in log lines. At debug level so prod typically masks, but worth pino-redacting `to` and `originalTo`.
- **L5. `crmInviteEmail`, `adminEmailChangeEmail`, `notificationDigestEmail` not in `TEMPLATE_KEYS`.** Means the admin can't customise their subjects at all — inconsistent with the rest. Either add them or document the omission.
- **L6. Hardcoded English copy.** Every template — buttons ("Sign in", "Activate account", "Set up your account"), greetings ("Dear …", "Hi …"), legal-ish boilerplate ("If you didn't request this …"). No i18n hook. Out-of-scope for v1 but flag for the Phase-7 cutover note in `project_email_ownership_at_cutover.md`.
- **L7. `SMTP_FROM` fallback in `sendEmail` builds `noreply@${env.SMTP_HOST}`.** If `SMTP_HOST` is `smtp.gmail.com` the From becomes `noreply@smtp.gmail.com` — invalid sender, instant SPF fail. Acceptable because production sets `SMTP_FROM` explicitly, but worth a `logger.warn` when this fallback is hit.
- **L8. Subject prefix when redirecting** (`index.ts:132-134`) — `[redirected from x@y]` appears verbatim and is fine in dev, but if `EMAIL_REDIRECT_TO` ever slips into prod (see C1) this is the only forensic trail.
---
## What's good
- All admin-supplied content (names, descriptions, custom messages, notes) is consistently `escapeHtml`-ed before interpolation. The only escape gaps are URLs (C2).
- Per-port branding shell is well-isolated; `getBrandingShell` falls back to defaults cleanly.
- SMTP timeouts are explicit (`SMTP_TIMEOUTS`, `index.ts:20-24`) — averts the BullMQ-slot starvation the comments warn about.
-`EMAIL_REDIRECT_TO` plumbing is consistent across `sendEmail`, `documenso-client`, `email-compose`, `webhooks` workers — when set, every outbound channel honours it.
---
## Suggested fix order
1. C1 (production guard for `EMAIL_REDIRECT_TO`)
2. C2 (URL escaping / scheme allowlist)
3. H3 (delete or wire up the dead Email Settings fields — fastest unblocker for admins)
- The signed-PDF iframe inside `documents/[id]/page.tsx` — when MinIO is
down, chrome-internal error renders in-place with no retry.
Wrap each in `WidgetErrorBoundary` with a sensible fallback.
### H5. New + public routes bypass `errorResponse()`
`grep "errorResponse"` shows 691 hits. The exceptions don't propagate
X-Request-Id and produce inconsistent shapes:
-`src/app/api/storage/[token]/route.ts` — bare `NextResponse.json({error:'Invalid or expired token'})`, no requestId.
-`src/app/api/public/website-inquiries/route.ts:75,122` — bare `{error:'Unauthorized'}` / `{error:'Unknown port'}`.
-`src/app/api/webhooks/documenso/route.ts:100` — `{ok:false, error:'Invalid secret'}` 200. Returning 200 is correct (no Documenso retry storms), but the literal string "Invalid secret" confirms the endpoint expects a secret. Drop the string.
`generateAndSignViaInApp` in `document-templates.ts` calls `documensoCreate(...)` and `documensoSend(...)`**without `portId`** (lines 831–843). `resolveCreds` then returns the global env triple `(DOCUMENSO_API_URL, DOCUMENSO_API_KEY, DOCUMENSO_API_VERSION)`.
- Per-port `apiVersion` ignored → a v2 port silently hits v1 endpoint paths (or vice versa); `createDocument`/`sendDocument` pick the wrong branch.
- Per-port `apiKey` ignored → auth fails on tenants whose key is only in `system_settings.documenso_api_key_override`.
-`redirectUrl` and `signingOrder` SEQUENTIAL/PARALLEL settings never plumbed — the in-app pathway passes no `meta` arg. Signers always land on Documenso's default thank-you page and v2 ports always sign PARALLEL regardless of admin choice.
Fix: thread `portId` and a `CreateDocumentMeta` built from `getPortDocumensoConfig(portId)` into both calls — mirror `generateAndSignViaDocumensoTemplate` at lines 894–910.
### C2. `handleDocumentCompleted` idempotency has a real cross-channel race
The early-return at `documents.service.ts:1110` (`if (doc.status === 'completed' && doc.signedFileId) return;`) is **necessary but not sufficient**. Two write paths can race:
1. Webhook receiver → `handleDocumentCompleted`.
2. Background poll worker `jobs/processors/documenso-poll.ts:63` → same call (same args).
The route-level `documentEvents.signatureHash` dedup only catches webhook→webhook repeats. It does **not** catch webhook + poll, because the poll worker bypasses the webhook entry point and has no signatureHash row. Both can:
Outcome: two `files` rows, two MinIO blobs; the second `UPDATE` overwrites `signedFileId`, orphaning the first row + blob (no DB pointer, never GC'd).
Fix: wrap gate-and-write in a transaction with `SELECT ... FOR UPDATE` on the documents row, or a pre-claim `UPDATE documents SET status='completing' WHERE id=? AND status != 'completed' RETURNING *` that atomically reserves the row.
`route.ts:264–266` catches every handler throw, logs, returns 200 (intentional — "always 200"). But a transient storage/DB failure inside `handleDocumentCompleted` is lost forever — Documenso records the event as delivered and never retries. Poll worker is the only safety net.
Fix: on handler throw, return non-200 so Documenso retries (bounded budget); or push the raw body onto a BullMQ replay queue.
---
## HIGH
### H1. Multi-berth `Berth Range` Documenso template field still pending
`buildDocumensoPayload` writes `formValues['Berth Range']: context.eoiBerthRange` (line 157), and `eoi-context.ts:128–135` populates it from `interest_berths.is_in_eoi_bundle=true` via `formatBerthRange()`. The **live Documenso v1 template does not yet have this field** (CLAUDE.md confirms). Documenso v1's `templates/{id}/generate-document` silently drops unknown `formValues` keys — multi-berth EOIs currently render with only the primary mooring in `Berth Number`.
The in-app pathway (`pdf/fill-eoi-form.ts:62–67`) fails loudly when its AcroForm field is missing; the Documenso pathway fails silently. Add a startup `GET /api/v1/templates/{id}` preflight that warns when `Berth Range` is absent.
### H2. `placeFields` v2 path is unverified against a live Documenso 2.x instance
`documenso-client.ts:636` has an explicit "must be confirmed against a live Documenso 2.x instance — top v2 risk" comment. Concerns:
Any port flipped to `apiVersion='v2'` using upload-and-place is rolling the dice until realapi run is green.
### H3. v1 fallback for CHECKBOX/DROPDOWN/RADIO is broken — silently
`fieldTypeNeedsMeta` permits CHECKBOX/DROPDOWN/RADIO. On v1, `placeFields` strips `fieldMeta` (lines 663–671 omit it) and v1's `/documents/{id}/fields` doesn't accept option metadata. A CHECKBOX placed on a v1 port renders as an unconfigured input with no options.
Code comment acknowledges ("falls back to blank-input behaviour"), but the placement UI gives no signal. Add a v1-aware preflight that disables these field types when `apiVersion='v1'`.
### H4. `sendDocument` v2 `redistribute` recipient scoping is unverified
`sendReminder` v2 (lines 391–407) ships `{ envelopeId, recipientIds: [signerId] }` to `/api/v2/envelope/redistribute`. The leading comment **contradicts** the body: "redistributes to all pending recipients on the envelope. Single-recipient targeting requires admin-side filtering."
If v2 ignores `recipientIds`, every "remind one signer" click resends to **everyone**, including already-completed signers — embarrassment risk on multi-signer EOIs. Realapi verification needed; reconcile comment with implementation either way.
---
## MEDIUM
### M1. `apiVersion='v1'` template-flow caveat correct but locks out v2 features
`generateDocumentFromTemplate` is hard-coded to `/api/v1/templates/{id}/generate-document` regardless of `apiVersion`. v2 instances accept this via backward-compat. Risk: a v2-native admin who built a template in the v2 UI may have **field IDs** but no stable **field names** — `formValues` keyed by name won't match. If Documenso drops v1 compat, every template-flow EOI breaks atomically. Plan now to capture per-template field-ID metadata in admin settings.
### M2. `getPageDimensions` cache + A4 assumption
`documenso-client.ts:597` returns `DEFAULT_PAGE_DIMENSIONS = { 595, 842 }` (A4 portrait, pt) unconditionally — the cache is dead code. Fine for the A4 EOI source PDF; for admin-uploaded contracts in Letter/A3/landscape, percent→pixel conversion is wrong by 5–30%, placing fields off-page or in the wrong band. Capture real page size via `pdf-lib` at upload time.
### M3. `normalizeDocument` recipient id collapses to `''` on missing fields
Line 75: `id: String(rec.recipientId ?? rec.id ?? '')`. When both keys are absent (malformed response), id becomes `''`; downstream maps keyed by recipient id collapse all phantoms into one bucket. Throw or filter when id is empty.
### M4. `applyPayloadRedirect` `/email$/i` regex is fragile
`documenso-client.ts:148` matches keys ending in `email`. A future field like `notificationEmailAddress` or `cc_email_2` would be missed and could leak past `EMAIL_REDIRECT_TO`. Either widen the heuristic, or declare email fields explicitly in `DocumensoTemplatePayload` and rewrite only those.
### M5. `voidDocument` 404-idempotency loses tenant signal
On 404, log + return silently. The local doc may still have `status='sent'`, so a retry re-attempts. Mostly benign — but set local `status='voided'` on 404 so DB converges with remote-not-found reality.
### M6. EOI hard-gate error code
`eoi-context.ts:206` produces `Cannot generate EOI - missing required client details: ...`. Labels are clean (good), but no structured code/field array — UI can't deep-link to the missing tab. Add `code: 'EOI_GATE'` + missing fields array.
### M7. Webhook `signatureHash` covers replay but not v2 timestamp drift
Confirmed body-sha256 dedups same-payload retries. If v2 ever varies `signedAt` on retry, the per-recipient `${signatureHash}:signed:${email}` keys differ → repeat processing. The per-recipient `document_events` index protects writes there, but `handleRecipientSigned` likely also advances interest stage — verify that side-effect is idempotent too.
---
## What's solid
- **`normalizeDocument` id↔documentId** symmetric; downstream consumes the legacy `id` form consistently — no stray reads of `documentId`/`recipientId`.
- **`canonicalizeEvent`** correctly maps `DOCUMENT_SIGNED` ↔ `document.signed` and routes v2 aliases (`RECIPIENT_SIGNED`, `RECIPIENT_VIEWED`) to v1 equivalents with a telemetry log line.
- **`handleDocumentCompleted` early-return** is the right shape for the common same-channel retry case. Cross-channel race (C2) is separate.
- **`eoi-context.eoiBerthRange`** plumbing correctly walks `interest_berths.is_in_eoi_bundle=true` and produces the compact range. Gap is template-side (H1).
- **SEQUENTIAL/PARALLEL `signingOrder`** correctly wired in `generateAndSignViaDocumensoTemplate` (`document-templates.ts:909`). Gap is the in-app pathway (C1).
- **`buildDocumensoPayload.meta.distributionMethod = 'NONE'`** — distribute invoked separately by `sendDocument`. Correct on both versions.
All 19 audit tasks finished. Every report is inlined above.
---
## Appendix: methodology + agent roster
Audit was run as a single `pn-crm-audit` Claude Code team. Each teammate was a separate Claude Opus 4.7 instance with read-only static-analysis scope (no file edits permitted by the brief). Time budget: 22 minutes per agent. Reports were written to `/tmp/audit-*.md` and consolidated here.
`src/lib/services/dashboard.service.ts:198-208` (`getHotDeals`) builds a CASE that references `'in_comms'` and `'deposit_10'`. The DB column `interests.pipeline_stage` stores `'in_communication'` and `'deposit_10pct'`. Both real stages fall through to `ELSE 0`, collapsing the rank ladder so any `eoi_sent` deal outranks every `in_communication`/`deposit_10pct` deal, and ordering inside the top tier becomes "newest updatedAt wins" instead of "furthest along."
The frontend mirror in `src/components/dashboard/hot-deals-card.tsx:26-36` (`STAGE_LABELS`) uses the same wrong keys (`deposit_10`, `in_comms`), so the badge for those two stages renders the raw enum string `deposit_10pct` / `in_communication` instead of "Deposit 10%" / "In Comms." Fix both files; prefer importing `STAGE_LABELS` from `@/lib/constants` rather than re-declaring it.
### C2. Revenue PDF "TOTAL COMPLETED REVENUE" silently includes **lost & cancelled** deals
`setInterestOutcome` in `interests.service.ts:919-943` forces `pipelineStage = 'completed'` for **every** outcome (won, lost\_\*, cancelled). `fetchRevenueData` in `report-generators.ts:126-140` then sums berth prices for `pipelineStage='completed' AND archivedAt IS NULL` with **no `outcome` filter**, and the PDF prints the result as `TOTAL COMPLETED REVENUE` (`revenue-report.ts:97`). Result: a marina with 1 won + 10 lost deals at €1M berths reports €11M completed revenue. Add `eq(interests.outcome, 'won')` to the `completedRevenue` query (and probably to the per-stage breakdown).
Postgres rejects a non-aggregated column without `GROUP BY` (`42803`). Either the pipeline PDF report has been crashing silently in the worker queue for any port with rows, or every run produces a single row that misses every stage but one. Add `.groupBy(interests.pipelineStage)`.
---
## HIGH
### H1. "Active interest" means **four different things** across surfaces
| `getKpis` / `getPipelineCounts` / `getRevenueForecast` (dashboard tiles + forecast) | `archivedAt IS NULL AND (outcome IS NULL OR outcome='won')` |
| `computePipelineFunnel` (analytics funnel) | same — but additionally bounded by `createdAt BETWEEN range` |
| `listInterestsForBoard` (kanban) — `interests.service.ts:194` | `archivedAt IS NULL` only ⇒ **lost & cancelled cards still appear** on the board (they all sit in the `completed` column because of C2) |
| `getHotDeals` | `archivedAt IS NULL AND outcome IS NULL` (also excludes **won** — intentional per comment but worth flagging) |
| `fetchPipelineData` / `fetchRevenueData` (PDF reports) | `archivedAt IS NULL` only ⇒ includes lost & cancelled |
| `computeRevenueBreakdown` (invoices) | unrelated definition — by invoice status |
A rep who reads "12 Active Deals" on the tile then opens the kanban can see 17 cards, because the kanban silently includes 5 lost deals routed to the `completed` column. Consolidate into a single `activeInterestsWhere(port)` helper and reuse everywhere.
### H2. Occupancy rate uses **two different sources**, same dashboard
-`getKpis` (KPI tile) + `fetchOccupancyData` (PDF) compute occupancy from `berths.status IN ('sold','under_offer')`.
-`computeOccupancyTimeline` (chart on the analytics page) computes occupancy from `berth_reservations` overlap with each day, with `total = COUNT(berths)`.
The two are unrelated: a berth marked `sold` with no active reservation contributes to the tile but not the timeline; a berth marked `available` with an active reservation contributes to the timeline but not the tile. Reps will see the tile read 64% and the chart's right-most point read 12% on the same day. Pick one definition (status-based is the documented one in CLAUDE.md) and align the timeline.
### H3. Revenue PDF stage breakdown is **unweighted**; dashboard forecast is weighted
`fetchRevenueData.stageRevenue` (`report-generators.ts:107-118`) does `SUM(berths.price)` per stage with no `pipeline_weights` multiplier. The dashboard `RevenueForecast` widget multiplies by `pipeline_weights[stage]`. So:
The two are reconcilable in principle but no rep will guess that. Either weight the PDF the same way, or rename the PDF column to "Berth Price by Stage (gross)".
`src/lib/constants.ts:68` (`STAGE_WEIGHTS`) and `src/components/admin/settings/settings-manager.tsx:76-86` hard-code the same object. Drift between the two means admins editing settings could see different defaults than the forecast actually uses. The settings form should `import { STAGE_WEIGHTS } from '@/lib/constants'` and spread it as `defaultValue`.
### H5. `getRevenueForecast` silently zeroes out stages with **missing weight keys**
`dashboard.service.ts:139` does `weights[stage] ?? 0`. If an admin saves `pipeline_weights` as `{ "in_comms": 0.2, ... }` (legacy key) or simply omits a stage, every active interest at the missing stage contributes €0 to the forecast — no warning, no fallback to `STAGE_WEIGHTS[stage]`. Validate the saved JSON against `PIPELINE_STAGES` at write time, OR fall back to the constant per-key (`weights[stage] ?? STAGE_WEIGHTS[stage]`).
---
## MEDIUM
### M1. Interests with no primary berth disappear from "pipeline value"
`getKpis` and `getRevenueForecast` use `INNER JOIN interest_berths ON isPrimary=true`. An interest without a primary-berth link (legitimate while the rep is still sourcing) contributes 0 to `pipelineValueUsd` and to `totalWeightedValue`, but is still counted in `activeInterests` and on the kanban. Mismatch between deal count and value. Surface a footnote (e.g. "5 deals not yet matched to a berth") or LEFT JOIN with a price-coalesce.
### M2. "Top Interests by Value" PDF includes lost deals
`fetchPipelineData.topInterestsRows` (lines 68-83) orders by `berths.price DESC NULLS LAST` with no outcome filter. A €4M lost deal will sit at the top of the report. Add `(outcome IS NULL OR outcome='won')`.
### M3. PDF stage order hardcoded inside both templates
`pipeline-report.ts:58-68` and `revenue-report.ts:55-65` redeclare the canonical stage order. Renaming a stage in `constants.ts` will leave the renamed stage appended to the "unknown stages" tail block instead of in its proper position. Import and iterate `PIPELINE_STAGES`.
### M4. `selectDistinct` in `pipelineValueUsd` is correct but fragile
`dashboard.service.ts:39-47``selectDistinct({ berthId, price })` happens to dedupe correctly because `berthId` is unique. If a future schema lets two `interest_berths` rows reference the same berth as primary (the partial unique index permits this if the other row has `isPrimary=false`), the join would still emit one row per primary-only match. Today's behaviour is fine; a comment in the code claims correctness but doesn't explain _why_. Add a one-line note tied to the partial unique index.
The query orders by `desc(rank), desc(updatedAt)` (`dashboard.service.ts:234`) but the card surfaces `last touched X ago` from `dateLastContact`. When a stage rank ties, the card with the most recent **edit** (rename, tag change, stage move) wins, not the most recent **contact**. Reps will be confused why an interest with 30-day-old `lastContact` sits above one with 2-day-old contact. Either order by `coalesce(dateLastContact, updatedAt)` or drop the "last touched" copy.
### M6. Source-conversion total includes archived-but-active deals only? No — also includes "still open"
`getSourceConversion` denominator is "every non-archived interest of that source" (`dashboard.service.ts:262`). For a source with 100 leads / 5 won / 0 lost / 95 still open, conversion = 5%. A source with 5 leads / 5 won shows 100%. The metric isn't wrong, but the description text "Won deals as a percentage of leads per source" implies a closed funnel; consider switching denominator to `won + lost` for the "true" rate, or rename the label.
3 CRITICAL bugs (hot-deals stage typos, lost-revenue mislabelled "completed", missing `GROUP BY` in pipeline PDF), 5 HIGH inconsistencies (active-deal definition splits 4 ways; occupancy split 2 ways; weighted vs unweighted revenue; duplicated weight defaults; silent zero-weighting), and 6 MEDIUM polish issues. The single most leveraged fix is consolidating one `activeInterestsWhere()` helper used by every surface, plus adding a `outcome='won'` filter to the revenue PDF and a `GROUP BY` to the pipeline PDF.
---
## 13. PDF + brand-asset correctness (pdf-auditor)
The same record is called **interest**, **lead**, **prospect**, and **deal** across surfaces. Sales reps and clients see all four within a single session.
-`src/components/berths/berth-interests-tab.tsx:44``hot_lead: 'Hot Lead'` (Title Case mismatch with sibling above).
-`src/components/dashboard/lead-source-chart.tsx` + `source-conversion-chart.tsx` widget title "Lead Source Attribution".
- "Prospect":
-`src/components/berths/berth-detail-header.tsx:~275` form label `Linked prospect (optional)` + helper `Link this status change to the prospect (interest) it relates to.` — explicitly parenthesises the canonical name as a synonym.
- Residential uses `prospect` as a _stage value_ (`Prospect` chip in `residential-clients-list.tsx`, residential-client tabs) — confusing because elsewhere "prospect" means the record itself.
-`src/components/berths/berth-tabs.tsx` tab label `Deal Documents`, API path `/api/v1/berths/[id]/deal-documents`.
-`src/components/clients/bulk-archive-wizard.tsx` placeholder `Why are you archiving this late-stage deal?` and `smart-archive-dialog.tsx` heading `Late-stage deal — confirmation required`.
Recommendation: pick one client-facing noun (the domain choice is `interest`; "deal" is fine as marketing/internal shorthand for _hot_ interests but should never appear in fields/labels). Rename `Deal Documents` → `Interest Documents`, "Linked prospect" → "Linked interest", `<dt>Lead</dt>` / `<h3>Lead</h3>` → `Buyer profile` or `Category`. Residential `prospect` is a stage so leave alone but consider renaming to `enquiry` or `new` to free up the word.
and line 65 the same pattern for `leadCategory`. Clients see `EOI: waiting for signatures`, `EOI: partially signed`, `hot lead`, etc. — the underscores are stripped but the enum vocabulary is not translated. "hot lead" exposed to the client is also a privacy/optics issue (we are telling the prospect we classified them).
Fix: add a `PORTAL_EOI_STATUS_LABEL` map (e.g. `waiting_for_signatures → "Awaiting your signature"`, `signed → "Signed"`); never render `leadCategory` in the portal at all.
### C3. Signing-status labels diverge across three surfaces
| `signing-progress.tsx``STATUS_LABELS` | Only `Pending / Signed / Declined` — missing Sent, Expired, Cancelled |
| `notification-digest.ts` email | `eoi_signed: 'EOI signed'`, `eoi_completed: 'EOI completed'` — "signed" vs "completed" used as if different events |
A user clicks a status pill in Documents Hub (shows `partially_signed`), opens the interest EOI tab (shows `Partially signed`), gets a toast that says `EOI fully signed`, and an email that says `EOI completed` — four phrasings for one document. Centralise via `src/lib/labels/document-status.ts` (already a pattern for `seed-data` etc.) and import everywhere.
-`Saving...` (ASCII three dots) vs `Saving…` (single ellipsis char) — both appear, ~50/50 split.
Decide: sentence case (`Save changes`) and standardise the loader as `Saving…` (Unicode ellipsis matches Prettier-friendly UTF-8 elsewhere in the codebase). The same form-pattern with a different casing in adjacent admin sections (user-form `Save changes` vs role-form `Save Changes`) is a likely playwright-visual diff source too.
### H2. "New X" vs "Create X" mismatch on the same surface
- Same pattern for Yacht, Expense, Company, Role, Tag, Template, Webhook, Port.
-`aria-label="New interest"` on `interest-list.tsx`, button text `Create Interest`.
Pick one verb per action lifecycle (`New …` for affordances, `Create …` for confirm, OR unify to a single `Create …` throughout). The current pattern teaches the user two words for the same action.
### H3. Public marketing form CTAs are all `Submit`
All five website forms (`website/components/pn/specific/website/{form,contact,berths-item,supplement-eoi,register,news-item}/form.vue`) use a bare `Submit` button. The matching confirmation email subjects say "Thank You for Your Interest" and the PDF title is "Expression of Interest". The CTA doesn't mention what the user is submitting.
Recommendation: replace with action-specific verbs — `Register interest`, `Send enquiry`, `Request a call back` (already used as helper text above the `Submit` on `berths-item/form.vue`). Loading state `Submitting form...` is also redundant — `Sending…` is shorter and matches the CRM email send loader.
### H4. EOI vs Expression of Interest abbreviation discipline
- **But** the portal interests page (`/portal/interests`) and the portal documents page header both say `EOIs` alongside `Expression of Interest` — `text-sm text-gray-500 mt-1`: "Your contracts, **EOIs**, and signed agreements".
- **Realtime toast** to staff says `EOI fully signed` — fine for staff but the same toast also fires for portal users if they have a session? (Worth verifying; if so, full form needed.)
- **PDF body** (`eoi-standard-inapp.ts:177`) introduces the abbreviation correctly: _This Expression of Interest (the "EOI")_ — good. Other PDFs (`interest-summary-template.ts`) use raw `EOI status: …` without that introduction.
Pattern: client-touching emails should land on one greeting (`Dear {name},`) and one sign-off (`Best regards, / The {portName} team`). The casual `Hi {name},` on the notification-digest is fine because it's internal to staff, but `Hello` on admin-email-change is just a third style for the same internal audience.
---
## MEDIUM
### M1. "Signing envelope" jargon leaks into a user-facing dialog
-`<option value="void_documenso">Void the signing envelope</option>`
"Envelope" is Documenso/DocuSign internal vocabulary. Replace with `Leave signing request pending` / `Cancel the signing request`. The Documenso admin page is OK to keep `envelope` (dev-facing settings).
`interest-stage-picker.tsx:179` shows `{overrideEffective ? 'Override stage' : 'Confirm'}`. The non-override label is too generic; users land on a stage-change dialog and the primary button just says "Confirm". Suggest `Move to {stage}` (parameterised) or `Update stage`.
`Saving...` (ASCII) vs `Saving…` (Unicode `…`). Easy global codemod; matters for Playwright visual diffs and for screen-reader pronunciation (three dots gets read out as "dot dot dot").
`Acknowledge` / `Dismiss` / `Mark complete` / `Resolve` (audit log) — four near-synonyms for "I dealt with it". Reminders use `Mark complete`, Alerts use `Acknowledge` + `Dismiss`. Acceptable if the semantics differ (acknowledge = seen, complete = done) but the current copy doesn't make that distinction clear.
### M5. "Hot Lead" / "Hot lead" casing within the same domain
5. Website: replace bare `Submit` on five forms with action-specific verbs. (H3)
6. Portal/email/PDF: drop bare `EOI` abbreviation in favour of `Expression of Interest`. (H4)
7. Standardise email greeting/sign-off pair per audience tier. (H5)
8. Replace `envelope` jargon in `smart-archive-dialog.tsx`. (M1)
Verified clean: `Inquiry` spelling consistent (American); `crm-invite.ts` use of "CRM" is staff-only and intentional; reports PDFs only use enum strings internally.
| `next` | 15.5.18 | 16.2.6 | App Router breaking changes in 16; defer until React/Next stabilise together |
| `eslint` + `eslint-config-next` | 9 / 15 | 10 / 16 | Lint-only; do alongside Next 16 |
| `zod` | 3.25.76 | 4.4.3 | **Wide blast radius** — every `src/lib/validators/*.ts` + `createTemplateSchema``VALID_MERGE_TOKENS` allow-list logic. Plan as its own task |
Scope: `Dockerfile`, `Dockerfile.dev`, `Dockerfile.worker`, `docker-compose.yml`, `docker-compose.prod.yml`, `docker-compose.dev.yml`, `next.config.ts`, `src/lib/env.ts`, `.env.example`, plus the entry points `src/server.ts` / `src/worker.ts` and health endpoints.
- Plus `storage/`, `playwright-report/`, `test-results/`, `tests/`, `scripts/`, screenshots, `.env*`.
Every `docker build` ships ~7.6 GB to the daemon. Worse, `Dockerfile.dev` and the builder stage of `Dockerfile` / `Dockerfile.worker` all do `COPY . .`, which means:
-`.env`, `.env.local`, `.env.dev` (if present) end up in build-layer history of the builder stage. The runner stage doesn't re-copy them, but intermediate layers are cacheable and pushable; a careless `--target builder` push leaks secrets.
- Local `node_modules` (built on macOS) get shipped to an Alpine builder and then ignored — silent waste, and `node_modules/sharp` darwin binaries collide with the musl install.
- Test snapshots / fixtures get baked into the trace.
`CLAUDE.md` is explicit: _"must be unset in production"_. The Zod schema in `src/lib/env.ts:41` accepts it unconditionally as `z.string().email().optional()`. If a staging `.env` leaks into a prod deploy (a very common ops mistake with the current `env_file: .env` setup — see M4), every outbound client email, EOI signing invite, and webhook delivery silently routes to the staging mailbox and the production user sees… nothing.
**Fix:** add a `superRefine` (or schema-level cross-check) that hard-fails when `NODE_ENV === 'production' && EMAIL_REDIRECT_TO` is set. Belt-and-braces: log a `logger.fatal` and `process.exit(1)` from `src/lib/email/index.ts` boot if the condition is reached.
### C3 — Custom server depends on `socket.io` that may not be in the standalone trace
`Dockerfile` runner stage copies only `.next/standalone/`, `.next/static/`, `public/`, and `dist/server.js` (renamed `server-custom.js`). There is no separate `pnpm install --prod` in the runner — every runtime dep must arrive via Next's `output: 'standalone'` tracer.
`src/server.ts` imports `@/lib/socket/server`, which `import { Server } from 'socket.io'` and `@socket.io/redis-adapter`. esbuild bundles `server.ts` with `--packages=external`, so at runtime `server-custom.js` does `require('socket.io')` against `/app/node_modules`. **Neither `socket.io` nor `@socket.io/redis-adapter` is in `next.config.ts:66 serverExternalPackages`**, and no Next route ever imports `@/lib/socket/server` (the socket server is only instantiated by the custom entry point), so the Next tracer has no reason to include them in `.next/standalone/node_modules`.
If this has been working in prod it's only because the packages get pulled in transitively via something Next does see. The dependency is invisible to the build system — a Next minor upgrade could drop them from the trace tomorrow.
**Fix:** add both to `serverExternalPackages`, _and_ extend `outputFileTracingIncludes` for the custom-server bundle, or COPY them explicitly into the runner from the deps stage:
`next.config.ts:31` — `script-src 'self' 'unsafe-inline'` regardless of `isProd`. Only `'unsafe-eval'` is gated. With `'unsafe-inline'` on, the entire XSS defence of CSP is defanged — any reflected/stored XSS still executes inline. The comment claims it's for Tailwind/Radix runtime styles, but those affect `style-src`, not `script-src`. Move to nonce or hash-based script policy in prod.
### H2 — `NEXT_PUBLIC_APP_URL` not in Zod schema, but baked at build time
`.env.example:67` lists `NEXT_PUBLIC_APP_URL` but `src/lib/env.ts` does not validate it. The builder stage runs `pnpm build` with `SKIP_ENV_VALIDATION=1`, so Next inlines an empty string when the var is missing. `src/providers/socket-provider.tsx:67` then runs `io(process.env.NEXT_PUBLIC_APP_URL!, {...})` → `io('', {...})` → browser falls back to `window.location.origin`, which silently _works_ in most cases but breaks the moment the CRM is fronted by a different origin than the socket gateway. `src/lib/auth/client.ts:12` has the same risk for the auth base URL during SSR.
**Fix:** add `NEXT_PUBLIC_APP_URL: z.string().url()` to the schema and pass it into the builder stage (via `--build-arg` + `ARG NEXT_PUBLIC_APP_URL` in `Dockerfile`). Drop `SKIP_ENV_VALIDATION=1` from the builder stage, or at least surface a build-time warning for missing `NEXT_PUBLIC_*` vars.
### H3 — `Dockerfile.dev` runs as root and re-installs on every layer rebuild
- No `USER` directive → dev container is root inside the bind-mounted `/app`. Any `pnpm dev`-spawned child can write to host-mounted files as root.
- Combined with C1 (no `.dockerignore`), the `COPY package.json pnpm-lock.yaml ./` followed by `pnpm dev` over a bind mount means the host `node_modules` shadow the in-container install on macOS (different platform), and dev images frequently break on `sharp`/`tesseract.js` until rebuilt.
**Fix:** create a `node` user (or reuse uid 1001), chown `/app`, drop privileges, and ship a working `.dockerignore` so the build context isn't 7.6 GB.
### H4 — `docker-compose.prod.yml` has no resource limits, no log rotation
`crm-app`, `crm-worker`, `postgres`, `redis` all run with default `unlimited` memory and the default `json-file` log driver. On a small VPS one runaway worker OOMs the host. The default log driver has no rotation, so disks fill silently. Add `deploy.resources.limits` (or top-level `mem_limit` in non-swarm mode) and `logging: driver: json-file, options: { max-size: "10m", max-file: "5" }` to every service.
### H5 — Compose healthcheck targets `localhost:3000`, but `env.PORT` is configurable
`docker-compose.prod.yml:45` and `.yml:43` hardcode `http://localhost:3000/api/health`. If a deploy sets `PORT=8080` via `.env`, the container listens on 8080, the healthcheck stays on 3000 → permanent "unhealthy" → restart loop. Either drop PORT from `env.ts` (the schema validates it but compose ignores it) or templatize the healthcheck (`wget … http://localhost:${PORT:-3000}/api/health`).
`Dockerfile.worker:38-39` checks `Redis.ping()`. A wedged BullMQ consumer (silent disconnect from the queue stream but TCP alive) passes this probe while jobs queue forever. Upgrade to read a sentinel BullMQ heartbeat key the worker writes on each job loop, or expose a tiny HTTP `/healthz` from the worker that asserts `queue.client.status === 'ready'` on the named queues.
### M2 — Worker re-installs deps in the runner stage
`Dockerfile.worker:31-32` does `pnpm install --frozen-lockfile --prod` in the runner — network round-trip on every build, even though the `deps` stage already has the full tree. Move to `COPY --from=deps /app/node_modules ./node_modules` then `pnpm prune --prod`, or use `pnpm deploy --prod --filter <pkg>`. Save ~30–60s per build and removes a network failure mode.
`socket.io`, `@socket.io/redis-adapter`, `imapflow`, `mailparser`, `pdf-lib`, `pdfme`, `sharp`, `tesseract.js` are all heavy native/CJS-leaning deps used server-side. Only 8 are listed. Anything missing risks bundling into the Next route trace (slower cold start, larger lambda/standalone size, possible runtime require failures for native bindings). Audit the import graph and add the rest.
### M4 — `env_file: .env` puts every secret into the container env
`docker-compose.prod.yml:36,59`. Anyone with `docker inspect` or `/proc/<pid>/environ` access on the host reads `BETTER_AUTH_SECRET`, `EMAIL_CREDENTIAL_KEY`, `DOCUMENSO_API_KEY`, `DOCUMENSO_WEBHOOK_SECRET` in plaintext. Switch to docker secrets (`/run/secrets/...`) or a sidecar mount and have `env.ts` read from file paths when the `_FILE` suffix is present.
Not in `.env.example`: `MULTI_NODE_DEPLOYMENT`, `WEBSITE_INTAKE_SECRET`, `EMAIL_REDIRECT_TO` (intentional per docs but the doc note exists; add a commented `# EMAIL_REDIRECT_TO=` line so devs _know_ it's an option), `DOCUMENSO_CLIENT_RECIPIENT_ID` / `DOCUMENSO_DEVELOPER_RECIPIENT_ID` / `DOCUMENSO_APPROVAL_RECIPIENT_ID` (env.ts has all three with defaults, but they should still be documented), `PORT`. The `EMAIL_CREDENTIAL_KEY` placeholder is 64 zeros — fine for dev but worth a comment that prod must rotate.
Both Dockerfiles use `node:20-alpine` (still LTS, but the `node:22-alpine` LTS is current). Neither installs `tini`/`dumb-init` — Node handles SIGTERM itself in these entrypoints so it's not broken, but if any child process is ever spawned (e.g. tesseract worker pool) zombie reaping is on Node. Cheap upgrade: `RUN apk add --no-cache tini && ENTRYPOINT ["/sbin/tini", "--"]`.
### M7 — `Dockerfile` runner has no HEALTHCHECK directive (only compose has one)
Image-level `HEALTHCHECK` makes the image self-describing — useful for non-compose orchestrators (swarm, nomad, k8s readinessProbe via `exec`). Add the same `wget … /api/health` line to the app Dockerfile as the worker Dockerfile already does for Redis.
`Dockerfile:14-15` sets `NEXT_TELEMETRY_DISABLED=1` + `SKIP_ENV_VALIDATION=1` but not `NODE_ENV`. `next.config.ts:3` branches on `isProd` for CSP — make this deterministic with `ENV NODE_ENV=production` above `RUN pnpm build`.
- **Cross-port leakage**: prevented at the entry point and the `feasible`
CTE; partial defense-in-depth gap at the aggregates layer (H1, M3).
---
## 19. Search relevance audit (search-auditor)
# Search relevance audit — task #19
**Scope:** `src/lib/services/search.service.ts`, `src/lib/services/search-nav-catalog.ts`, `src/components/search/command-search.tsx`, `src/hooks/use-search.ts`, plus the `resolve-id` route used by paste detection.
**Method:** Read each file in full, traced the ranking formulas, simulated the three test queries against `scoreEntry`, audited the graph-expansion merge for permission leakage, and spot-checked the catalog for duplicates.
---
## Spot-check results (the three required queries)
All three pass — but with a duplicate-result wrinkle (see HIGH-2).
Runner-ups for `ai`: System Settings (35, `ai interest scoring` keyword prefix) and Profile (20, "avatar" substring). Acceptable noise floor.
---
## CRITICAL
None. The system is solid overall — sanitization is correct, port isolation is consistent, the affinity boost is bounded, and paste detection is port-scoped via the resolve-id endpoint (good — prevents cross-tenant navigation on super-admin paste).
`search()` (line 1809–1865) gates each direct-match bucket via `can(opts, '<x>.view')`. Then `expandGraph` runs unconditionally on whichever direct matches survived, and its output is pushed into `mergedClients` / `mergedInterests` / `mergedYachts` / `mergedCompanies` / `mergedBerths` via `mergeWithExpansion` (lines 1911–1915) — **without re-checking the destination bucket's permission**.
Concrete leak: a user with `berths.view` but **no**`interests.view` who searches `A12`:
- expansion: `interestsFromBerths` → populates `expandedInterests` → merged into `mergedInterests` → returned to the client
- The dropdown renders rows with the client's full name + pipeline stage from interests the user cannot otherwise read
Similar leaks: berth name via yacht-direct match → `expandedBerths`; client names via company-direct match → `expandedClients`; etc.
Fix: gate the expansion writes — only push `expanded.X` into `mergedX` when `can(opts, '<X>.view')`. Cleanest: pass the `can(...)` results into `mergeWithExpansion` as a "destination allowed" boolean.
### HIGH-2 — Six catalog labels are duplicated under different hrefs
The catalog has both `/settings/X` and `/admin/X` entries with near-identical labels, so common queries return two visually-similar rows pointing at different pages:
For users who have both `manage_settings` permissions, the dropdown shows two indistinguishable rows. Recommendation: either (a) collapse to one canonical entry per concept, or (b) disambiguate the label suffix (e.g., "Email accounts (admin)" vs "Email accounts (self-serve)"). The duplication reflects the underlying double-page structure, which deserves its own product decision.
### HIGH-3 — `looksLikeEmail` / `wantPhone` are computed then discarded
Lines 1804–1807 compute `wantEmail` and `wantPhone`, then lines 1885–1886 do `void wantEmail; void wantPhone;` with a TODO-style comment. Dead code paid for on every request. Either delete or wire it into the bucket reordering the comment promises.
---
## MEDIUM
### M-1 — `applyAffinity` re-sorts AFTER `mergeWithExpansion`, breaking the direct-first guarantee
`mergeWithExpansion` (line 1754) carefully puts direct matches before expansion rows. Then `apply()` (line 1905) re-sorts the merged list by recently-touched membership — a recently-touched related-via row can leapfrog a direct (non-touched) match. Either intentional (and should be documented) or a bug (and the merge ordering is wasted work). The current behavior surprises me: I expect direct matches to always win at the top.
Clients section uses tsvector OR ILIKE; berths section uses `b.mooring_number % ${query}` (pg_trgm operator with the default 0.3 threshold). Berths are short codes — trigram on them is unreliable ("A12" trigram similarity to "B12" is ~0.5, both surface). Standardize: berths should match via prefix only (consistent with the in-port `searchBerths`).
### M-3 — `searchNotes` interest-branch source_label drops when no primary berth
Line 1166: `b.mooring_number AS source_label` is null when the interest has no primary berth, so the row's `sourceLabel` falls back to the generic "Interest" via `labelForSource`. The interest's client name would be a far more useful label (the interests bucket uses it). Patch: COALESCE with the client name via an extra JOIN.
`INVOICE_RE = /^INV-\d{6}-\d+$/i` (line 92) assumes the legacy 6-digit prefix. The resolve-id endpoint also accepts `invoice_number` lookup, so non-matching shapes silently fall through to free-text search. Not a security issue, but if invoice numbering changes the paste shortcut breaks invisibly. Consider expanding to `/^INV[-_/].+$/i` and letting the resolve-id endpoint be the source of truth.
### M-5 — Non-ASCII characters in names are stripped by tsquery sanitizer
`buildPrefixTsquery` (line 278) strips `[^a-z0-9_]`, so `Šibenik`, `Łukasz`, `Müller` all reduce to empty tokens. The trigram fallback `similarity()` saves most of these (it's diacritic-tolerant for >0.3 similarity), but exact-prefix matching on accented names is lost. For Croatian / Polish / German tenant names this matters. Consider `unaccent()` before sanitization or relax the regex to `\p{L}`.
### M-6 — `expandGraph` issues N+1-style queries for each direct ID set
The `LIMIT ${perBucketCap * direct.<X>Ids.length}` pattern (e.g., line 1387, 1463, 1486) scales the row cap by direct-match count. With limit=5 and 5 direct berth matches, that's 25 expansion rows fetched, then merged into the same 10-row `limit * 2` cap downstream — most fetched rows are thrown away. Minor cost; cap globally instead.
### M-7 — `searchDocuments` JOIN on `document_signers` has no port_id filter
Defense-in-depth: `ds.signer_email` ILIKE match is filtered through `d.port_id`, but the JOIN itself doesn't carry the port filter. Documents are FK-scoped, so no leak today, but the recommender pattern in this codebase (per CLAUDE.md) says "defense-in-depth port_id filter at every join." Apply the same here.
### M-8 — `import()` of `searchNavCatalog` inside `search()` is sync wrapped in two `await`s
Line 1867 — `await Promise.resolve((await import('@/lib/services/search-nav-catalog')).searchNavCatalog(...))`. The dynamic import is fine (avoids a circular dep), but `Promise.resolve` wrapping a sync result then awaiting it is dead ceremony. Inline or `await import(...).then(...)`.
`BUCKETS` in `command-search.tsx` (lines 60–81) — confirmed. Notes is index 14, Navigation is index 15. `buildFlatRows` preserves this order, and the comments at lines 75–79 and 1135–1138 document the rationale.
---
## What works well
-`scoreEntry` ladder (label-exact 100 → label-prefix 80 → label-substring 60 → kw-exact 50 → kw-prefix 35 → kw-substring 20) is correct and matches the spec.
- Paste detection: regex narrowness is fine because resolve-id is port-scoped and the fallback is normal search.
- The `NEVER_TSQUERY` / `NEVER_PHONE` sentinels (line 385–386) correctly avoid Postgres-evaluation-order surprises that would otherwise break NULL guards in WHERE.
-`searchBerths` exact-match short-circuit (line 757) is the right UX call — typing "A1" when A1 exists should not also dump A10–A19.
- Catalog `requires` is permission-gated correctly and `searchNavCatalog` respects both `requires` and `superAdminOnly`.
-`mergeWithExpansion` uses a `Set` dedupe — direct match wins, no duplicate rows.
-`applyAffinity` is stable wrt original order (line 327) when the touched-set is empty.
**Good news up front**: no `@ts-ignore` / `@ts-expect-error` anywhere, and no `$inferSelect` type leaked through the API boundary as a public response contract. Service return shapes go through `{ data }` envelopes; drizzle row types stay internal.
---
## CRITICAL
### 1. `tx: any` in client-restore service — bypasses Drizzle's transaction type contract
This parameter receives a Drizzle transaction client and threads writes through 12+ downstream tables in a multi-step restore. A typo'd table or wrong column type goes undetected at compile time. Type as `Parameters<typeof db.transaction>[0]` (see `src/lib/db/utils.ts:17` for the same shape applied via `as unknown as`).
### 2. `useQuery<any>` + `apiFetch<{ data: any }>` on berth detail page
Three escape hatches stacked on the highest-traffic detail page. Every field access downstream is unchecked — a service-side rename to `mooringNumber` → `mooring_number` would silently render `undefined`. Replace with a `BerthDetailResponse` type co-located with the service.
### 3. Portal-auth and public routes bypass `parseBody`
CLAUDE.md mandates `parseBody` so 400 errors have field-level shape the toast hook recognizes. ZodErrors from `schema.parse` after raw `req.json()` become generic 500s. Custom-fields one is justified; the other 9 are not.
---
## HIGH
### 4. `mergePerms` double-cast in new permission-overrides route
Comment acknowledges this duplicates `withAuth`'s `deepMerge`. Either reuse `deepMerge` from `helpers.ts` (lines 202–205, 234–237 already use the same pattern) or extract a typed helper `mergePermsTyped(base, patch): RolePermissions`. Two implementations of permission merge is a divergence risk.
### 5. Audit-log `as unknown as Record<string, unknown>` epidemic
Wide repetition of the same widening cast is a smell — every service does the same dance to fit Drizzle row types into the audit JSONB column. **Fix**: introduce `toAuditJson<T>(row: T): Record<string, unknown>` once in `src/lib/services/audit.ts` (same pattern `gdpr-bundle-builder.ts` already uses — `toJsonRow`, line 152 comment explicitly cites avoiding this). Removes 21 unsafe casts in one shot.
### 6. `next/typedRoutes` defeated by 49 `as any` href casts
`router.push(..)` and `<Link href={..}>` with template-literal dynamic URLs get widened to `string`, which isn't assignable to `Route<string>`. Components compensate via `as any` (49 sites) or `as Route` (17 sites). Hotspots: `command-search.tsx` (10), `topbar.tsx` (10), `user-menu.tsx` (8), `reservation-list.tsx` (6), residential lists/headers (8).
This nullifies the value of `experimental.typedRoutes` everywhere it matters most — dynamic navigation in shells, search, and detail headers. **Fix**: introduce `route(path: string): Route` helper in `src/lib/routes.ts` that does the cast in one audited place; ban `as any`/`as Route` for href via ESLint rule. Bonus: makes it possible to migrate to a real typed-routes wrapper later.
### 7. `RolePermissions` ↔ `Record<string, unknown>` round-trip in withAuth chain
`src/lib/api/helpers.ts:203, 235` — two layers of permission merge cast both directions to satisfy `deepMerge`'s untyped signature. `deepMerge` should be generic over `<T extends Record<string, unknown>>` and accept `RolePermissions` directly. Same problem as #4; same fix.
---
## MEDIUM
### 8. `Record<string, unknown>` JSONB writes without zod re-parse at write time
-`ocr-config.service.ts:79, 85` — `value: value as unknown as Record<string, unknown>`. Upstream zod parse exists, so safe in practice, but the cast hides the relationship.
-`ai-budget.service.ts:88, 94` — same pattern.
-`me/route.ts:148–168` — **good model**: explicit `ALLOWED_PREF_KEYS` allow-list + 8KB size cap + zod via `parseBody`. Use as the template for the other two.
-`components/admin/settings/settings-manager.tsx:237, 250` and `settings-form-card.tsx:79–93` — client-side `Record<string, unknown>` state. Admin-only surfaces, low risk, but no per-key shape check before PUT.
The `query.sort` zod schema should already enum-restrict sort keys to actual columns; if so, the inner `as keyof typeof invoices` is redundant. If `query.sort` is a free string, this is also a SQL-shape risk surface (mitigated only because Drizzle column proxy will throw on unknown keys). Verify the validator enum is exhaustive.
### 10. Template-preview accepts arbitrary content as TipTap
Admin-gated, so blast radius limited, but the renderer downstream assumes well-formed TipTap JSON. Add a minimal `tipTapNodeSchema` zod check at the boundary — a malformed node tree would otherwise throw deep in the renderer with a useless stack trace.
Type system gap is genuine (Node Readable ↔ Web ReadableStream don't have a structural match in lib.dom + @types/node). Centralize in `src/lib/storage/stream-bridge.ts` with named helpers `toWebStream(readable)` / `toNodeStream(web)` — removes the casts from feature code.
`brochures.service.ts:255` and `berth-pdf.service.ts:350` reach into stream internals because the storage backend's return type doesn't expose `destroy`. Add `destroy?(): void` to the `StorageBackend.get()` return type so cleanup is part of the contract.
### 13. `as unknown as string` for pdfme BLANK_PDF sentinel
9 PDF templates carry `basePdf: 'BLANK_PDF' as unknown as string`. This is a known pdfme upstream type-def bug — the string literal `'BLANK_PDF'` is accepted at runtime but typed as `Uint8Array | string`. Wrap once: `const BLANK_PDF = 'BLANK_PDF' as unknown as string;` exported from `src/lib/pdf/constants.ts`. Removes 9 casts.
`src/lib/dedup/phone-parse.ts:25` — `const metadata: any = require(...)` for `libphonenumber-js/metadata.min.json`. CommonJS interop hack; replace with `import metadata from 'libphonenumber-js/metadata.min.json'` + `resolveJsonModule: true` (already on in `tsconfig.json`).
---
## Drizzle leak check — clean
Searched for `$inferSelect` / `$inferInsert` exports crossing the API boundary: **zero hits in src/**. Services return Drizzle row types internally, but every API route wraps them in `{ data }` envelopes (confirmed by spot-check across invoices, berths, clients, documents). The `Record<string, unknown>` widenings flagged above happen at write time into JSONB columns, not at read time across the API surface. No PII columns or internal-only fields slip through.
Better Auth's `sendResetPassword` (`src/lib/auth/index.ts:73`) is configured with no `onPasswordReset` / `revokeAllSessions` hook; the same is true for `resetPassword` in `portal-auth.service.ts:428`. Outcome: a compromised cookie keeps working forever after the legitimate owner does the "forgot password" dance. This is the canonical "session-bumping on reset" guarantee users assume and we're not delivering it. Add a step that deletes every row in `sessions` (CRM) and `portal_auth_tokens` + active `portal_sessions` (portal) for the affected user inside the same transaction that writes the new password hash.
### C2 — Disabled CRM user retains an active session cookie
-`auth.api.signInEmail` itself doesn't know about `isActive` — a disabled user can still complete `/login` and be redirected to `/dashboard`, where every API call then 403s.
- Setting `isActive=false` in `updateUser` (`users.service.ts:227`) never deletes the existing `sessions` row, so an already-logged-in disabled user keeps every page that doesn't hit `/api/v1` working, and any cached SSR page loads.
Fix: (a) on signIn add a profile lookup and reject before issuing the cookie; (b) on `isActive=false` flip, delete from `sessions` for that userId; (c) middleware should treat a 403 from an API as a global redirect to `/login?reason=disabled`.
---
## HIGH
### H1 — No dedicated "this link expired / already used / your account is disabled" landing pages
Every token failure today is surfaced as a toast on a still-functional form, or as a 400 JSON error that the user only sees if they actually submit the form.
-`set-password/page.tsx` (CRM) handles `!token` (the "Link is missing or invalid" branch, line 73-88) but does NOT distinguish "token present but expired" from "token present but already used" — both surface as a `toast.error(body.message)` and leave the form interactive, inviting an infinite retry loop.
- The portal `password-set-form.tsx` is identical (line 63-67): expired/used tokens render as a red `<p>` under the form.
- There is no `/account-disabled` page; the user just sees `403 Account disabled` text from the JSON response in DevTools and gets stuck on `/dashboard` rendering nothing (or rendered SSR shell with broken API calls).
Recommend: a single `<TokenStateMessage state="expired" | "used" | "invalid" />` component that the page server-renders by doing a HEAD-style `validate` call on mount, plus a `/disabled` route the middleware redirects to.
### H2 — Email-change `/settings?emailChange=confirmed|cancelled` query param is never consumed
The confirm/cancel redirect URLs at `api/v1/me/email/{confirm,cancel}/[token]/route.ts:71/50` set `?emailChange=confirmed|cancelled`. Grep shows ZERO consumer in `src/app` or `src/components`. The redirect succeeds, but the user lands on the bare `/settings` page with no banner, no toast, no confirmation — for a security-sensitive action this looks broken / makes users wonder if it took. Wire a banner in `user-settings.tsx` keyed off `useSearchParams().get('emailChange')`.
### H3 — Cancel-email-change link is GET-only with no friction
`api/v1/me/email/cancel/[token]/route.ts` is a one-click GET that wipes the pending change. Gmail/Outlook link prefetchers, antivirus URL scanners, and corporate proxies will auto-fetch links and cancel a legitimate request without the user ever clicking. Pattern for safety: GET renders a confirmation page (`Are you sure?` button), POST executes. Same fix needed on `confirm` if a link-scanner could pre-confirm an attacker's address-change before the real user sees the cancel link.
### H4 — `set-password` (CRM) success path has no auto-sign-in
`set-password/page.tsx:64` toasts "Password set successfully" then routes to `/login`. The user has to type their email and the password they just chose, again. For invite flows this is the worst conversion point. Either (a) auto-sign-in via `auth.api.signInEmail` after the consume call returns, or (b) at minimum prefill the email field on `/login`. (Portal's `activate` flow has the same problem in `password-set-form.tsx:97`).
### H5 — Reset-password expiry shows no time estimate; users hit "expired" cold
CRM `(auth)/reset-password/page.tsx:62` says "we have sent a password reset link" with no TTL. Better Auth default reset token expiry is 1 hour (the email body on line 79 mentions "expires in 1 hour" but the success page doesn't echo this). Portal forgot-password (`forgot-password/page.tsx:43`) correctly says "expires in 30 minutes". Make the CRM message say "Link expires in 1 hour" so users at the airport know whether to wait.
### H6 — `resolve-identifier` returns 429 with `{ email: '' }` which bypasses the synthetic miss path
`api/auth/resolve-identifier/route.ts:56` returns `{ email: '' }` on rate-limit. Client (`login/page.tsx:56`) does `payload.email?.trim() || identifier` — so the original username (without `@`) is passed into `authClient.signIn.email`. Better Auth rejects it as "invalid email format" instead of "invalid credentials", which is a distinguishably-different error from the normal miss case and re-opens the enumeration channel the synthetic-email defence was built to close. Return `{ email: syntheticEmail(raw) }` on the 429 path too (status code can stay 429).
`login/page.tsx:65` uses `result.error.message ?? 'Invalid credentials'`. Better Auth surfaces strings like "User not found" / "Invalid password" / "Email or password is invalid" depending on the path — the first two are an enumeration leak that bypasses the resolve-identifier defence. Always overwrite to a fixed `'Email or password is incorrect.'` and only log the underlying reason server-side.
### M2 — Portal sign-in error message is friendlier than CRM
Portal: `'Invalid email or password'`. CRM: raw Better Auth message OR `'Invalid credentials'`. Unify on "Email or password is incorrect" everywhere (matches CRM `(auth)/login/page.tsx:65` and portal `(portal)/portal/login/page.tsx:37`) — the CRM phrasing "Invalid credentials" is jargon.
CRM `set-password/page.tsx` requires min 9 chars (line 16). Portal `password-set-form.tsx` also 9 (line 23). But the activation/CRM invite TTL diverges silently: CRM invite = 72h (`crm-invite.service.ts:17`), portal activation = 72h (`portal-auth.service.ts:25`), portal reset = 30min, CRM reset = 60min. The "request a new link" copy in the invalid-token branch should embed the actual TTL so admins debugging "why doesn't this work" don't have to read the schema.
### M4 — `set-password` (CRM) error fallback is inconsistent shape
`(auth)/set-password/page.tsx:60` reads `body.message ?? body.error` — but `api/auth/set-password/route.ts` uses `errorResponse(err)` which emits `{ error }`. The `message` key is dead code, fine, but the legacy comment on `set-password/route.ts:24` says envelopes were normalised in commit "auditor-F §32" — the page should match: `body.error ?? 'Failed to set password.'`.
### M5 — Portal forgot-password 30-min TTL is short for international clients
30 minutes is aggressive when emails routinely sit in spam quarantine for 5-15 minutes before clearing. CRM reset's 60min is a sensible floor. Either lift to 60min or surface the 30min countdown more aggressively in the email + landing page.
`set-password/page.tsx:143` falls back to `<BrandedAuthShell>{null}</BrandedAuthShell>` — a flash of empty branded card while `useSearchParams` resolves. Replace with a skeleton or "Verifying link…" microcopy; the empty state reads as "page broken" for ~100ms on slow networks.
### M7 — `/portal/activate` Suspense fallback is unbranded grey div
`portal/activate/page.tsx:8` falls back to a plain `Loading…` div — jarring after the branded email. Mirror the CRM `set-password` pattern with `<BrandedAuthShell>`. Same on `portal/reset-password/page.tsx:8`.
### M8 — "Request a new link" target on portal `set-password` invalid-token is wrong for activation flow
`password-set-form.tsx:86` always points to `/portal/forgot-password`. For activation the user has no password yet — `/portal/forgot-password` returns the silent 200 and the admin has to manually `resendActivation`. Branch on `endpoint`, or give portal users a self-service "Resend activation".
Better Auth session `expiresIn: 24h` (`auth/index.ts:94`); portal token also 24h. No checkbox to shorten on a shared device, no copy saying so. Add a session-only cookie path.
### M10 — Portal login `next` param is unvalidated
`portal/login/page.tsx:42`: `router.replace(next as never)` where `next = search.get('next')`. Open redirect: `/portal/login?next=https://evil.example` navigates cross-site after sign-in. Validate `next.startsWith('/portal/')` before using.
Grep `serviceWorker|navigator.serviceWorker|workbox|next-pwa` returns nothing across `src/` + `public/`. The per-port manifest declares `display: 'standalone'` and the scanner's whole product premise is "rep walks the marina with a phone capturing receipts", i.e. exactly the situation where Wi-Fi drops to nothing between pontoons. Consequences:
- iOS Add-to-Home Screen installs succeed but cold-launch with no signal fails at the first network call (Next.js page chunks 404 in WebView).
- The OCR + upload + create-expense chain in `ScanShell` (`src/components/scan/scan-shell.tsx`) has no offline-queue / retry. `kind: 'error'` is rendered and the only artifact is the in-memory blob — closing the PWA loses the photo and the manual-typed fields.
- Android Chrome will refuse to fire `beforeinstallprompt` without a service worker, so the install prompt never auto-surfaces.
Fix (in priority order): (1) ship a minimal Workbox/`next-pwa` SW that precaches the scanner route + Tesseract WASM + lucide icons, (2) wrap the expense submit in an outbox (IndexedDB queue → background sync), (3) capture `beforeinstallprompt` and surface an "Install" CTA inside the idle-state scanner card.
### H2. Two manifests overlap — root `/manifest.json` and dynamic `/[portSlug]/scan/manifest.webmanifest`
- Root layout (`src/app/layout.tsx:47`) declares `manifest: '/manifest.json'` with `start_url: '/'`, `theme_color: #0f172a` (slate-900). Root viewport says `themeColor: '#1e2844'` (navy). Two different theme colors → Chrome will pick the `<meta name="theme-color">` from `<head>` (navy) but the manifest install splash will use `#0f172a`. Cosmetic mismatch on install.
- Scanner manifest overrides scope to `/<portSlug>/scan` with `theme_color: #3a7bc8` (brand blue) and viewport `themeColor: '#3a7bc8'` — internally consistent. ✓
- Issue: if a rep visits `/<portSlug>/dashboard` and hits "Add to Home Screen" (rare but possible), they get a PWA whose `start_url` is `/` which redirects to `/login` on every cold-launch because the root `<head>` resolves the unscoped manifest first. There is no `<link rel="manifest">` swap between the two surfaces; Next.js's `generateMetadata` on the scanner route DOES override the root metadata (verified at `src/app/(scanner)/[portSlug]/scan/layout.tsx:28`), but root `/manifest.json` still defines a competing PWA.
Fix: either narrow the root manifest's `scope` and `start_url` to `/login` (so non-scanner installs land on auth), or remove root `manifest:` and lean solely on the per-port scoped scanner manifest. Add `start_url: '/<portSlug>/dashboard'` per-port via a second dynamic manifest for the main app, if installable main-app is even desired.
### H3. iOS standalone status-bar / safe-area mismatch in the scanner
- Scanner layout declares `appleWebApp.statusBarStyle: 'default'` (`src/app/(scanner)/[portSlug]/scan/layout.tsx:32`) — that's the white-bar-with-black-text style that iOS draws OPAQUELY above the WebView, NOT under it.
-`viewport.viewportFit: 'cover'` is set (line 46) which tells iOS to let content extend under safe areas.
-`ScanShell` (`src/components/scan/scan-shell.tsx:449`) renders `<main className="mx-auto ... min-h-[100dvh] w-full max-w-xl ... px-4 py-6 sm:py-10">` — **no `pt-safe-top`, no `pb-safe-bottom`, no `safe-left/safe-right`**.
- Result on iPhone 14/15 with home indicator + standalone install: the "Capture receipt" / "Save expense" buttons sit flush against (or under) the home-indicator stripe. The brand logo at the top is fine because `py-6 sm:py-10` happens to clear the notch — by accident, not by design.
Fix: add `pb-[calc(env(safe-area-inset-bottom)+1rem)]` to `<main>`, switch `statusBarStyle` to `'black-translucent'` so the brand-blue theme paints over the status area (or to `'default'` AND remove `viewportFit: 'cover'`), and add `pl-safe-left pr-safe-right` for landscape edge-case.
### H4. Dashboard mobile shell uses `min-h-screen` (`100vh`) instead of `100dvh`
`src/components/layout/mobile/mobile-layout.tsx:24,29` uses `min-h-screen` twice. On iOS Safari (not standalone) `100vh` is the LARGE viewport height (URL bar collapsed), so on first paint the page renders ~75–100px taller than visible. The bottom tab bar is `position: fixed` so it lands correctly, but `<main>`'s `min-h-screen` means content scrolls below the visible viewport on initial load — reps see a blank strip past the tab bar until the URL bar collapses on first scroll.
Fix: swap both `min-h-screen` for `min-h-[100dvh]` (Tailwind 3 supports dynamic viewport units). The scanner layout already does this correctly (`src/app/(scanner)/[portSlug]/scan/layout.tsx:68`).
## MEDIUM
### M1. Touch targets below 44pt in the mobile search overlay
- "Cancel" button (line 273) is plain text — no min-height, hit-area ≈ 16px tall. Thumb-prone position next to the keyboard.
- Clear-X button (line 260) is `size-7` = 28px. Below Apple HIG 44pt.
- Bucket chips (line 344) are `px-3 py-1.5 text-xs` → ~28px tall. Apple HIG 44pt fail; they're scrollable so misses are recoverable, but each chip needs `min-h-[44px]` or a transparent expanded hit-box (`before:absolute before:inset-0 before:-my-2`).
### M2. Inline-editable-field hit-areas too small for marina-glove use
`src/components/shared/inline-editable-field.tsx:133,172,257` uses `h-8` (32px) and `h-7` (28px) for the edit-mode inputs and select triggers. Detail pages on mobile share this pattern. Apple HIG fail; reps with wet/salty fingers on a pontoon will mis-tap. Bump to `h-11` (44px) on mobile or guard with a `min-h-[44px] md:h-8` mobile-first override.
### M3. `visualViewport.offsetTop` ignored in search overlay positioning
`src/components/search/mobile-search-overlay.tsx:76–86` subscribes to `visualViewport.resize` + `scroll` and reads `vv.height`. The drawer uses `top: 12px` + computed height. But `vv.offsetTop` (the visual-viewport's vertical offset within the layout viewport) is not consulted. On iOS Safari with keyboard up + rubber-band scroll, the visual viewport can shift relative to layout; the drawer's `top: 12px` is layout-viewport-relative, so the top of the drawer can briefly clip up under the URL/status bar. Minor visual artifact; only affects scrolled-during-typing states.
Fix: `top: ${(vv?.offsetTop ?? 0) + 12}px`.
### M4. Mobile bottom tabs lack `safe-left` / `safe-right` insets
`src/components/layout/mobile/mobile-bottom-tabs.tsx:42–47` uses `pb-safe-bottom` only. The dynamic manifest forces `orientation: 'portrait'` ONLY when installed as a PWA. In Safari (pre-install) on iPhone landscape, the bottom tab bar tucks under the notch. Add `pl-safe-left pr-safe-right` (Tailwind `pl-safe-left` resolves to `padding-left: env(safe-area-inset-left)`).
`project_pwa_assets_pending.md` claims icons must be added; all four exist in `/public` (icon-192 = 688B, icon-512 = 2411B, 512-maskable = 2411B, apple-touch = 654B; dated 2026-05-03). Memory note is stale — delete it. **However**: 688B / 2411B is small for a real branded PWA icon — these look like placeholders. Swap in production artwork before launch.
### M6. apple-touch-icon at `/apple-touch-icon.png` not referenced by the scanner manifest
The root metadata icons block (`src/app/layout.tsx:40–46`) declares `apple: '/apple-touch-icon.png'` (180×180). The scanner layout only sets `manifest:` + `appleWebApp` — it inherits the root `icons.apple` because Next.js does shallow-merge of metadata. ✓ but only because of inheritance; explicit confirmation in a comment would prevent future regressions if someone overrides `icons:` in the scanner layout.
### M7. No `apple-mobile-web-app-status-bar-style` mismatch detection between routes
Root layout: `'black-translucent'` (matches navy theme + safe-area inset). Scanner: `'default'` (white opaque bar). When a rep navigates from `/scan` into the main CRM via a deep link inside the same PWA install, iOS uses the install-time status bar style and ignores per-page overrides — so depending on which surface they installed FROM, every other surface looks wrong. Pick one style and apply globally; recommend `'black-translucent'` plus consistent safe-area-inset usage on every shell.
### M8. Vaul drawer `repositionInputs={false}` defaults are correct, but iOS keyboard layoutViewport vs visualViewport edge case
`src/components/shared/drawer.tsx:20–22` defaults `shouldScaleBackground: false` and `repositionInputs: false`. The comments in `mobile-search-overlay.tsx:106–118` describe the iOS reasoning correctly. Verified ✓. However, the MoreSheet's `<DrawerContent>` uses default `bottom: 0` anchoring (no visualViewport-based height override). If MoreSheet ever gains a text input, it'll exhibit the same scroll-then-jump the search overlay had to special-case. Currently MoreSheet is link tiles only — non-issue unless inputs are added.
### M9. No `<NoScript>` or offline fallback page anywhere
If the scanner PWA cold-launches with no network and no service worker (H1), Next.js's standalone-mode router will fail-soft to a blank screen. There is no `not-found.tsx`, `error.tsx`, or `offline.tsx` in `src/app/(scanner)/[portSlug]/scan/`. Goes hand-in-hand with H1.
### M10. The legacy `/expenses/scan` page coexists with the new `/scan` PWA flow
`src/app/(dashboard)/[portSlug]/expenses/scan/page.tsx` is a desktop-flavored scan-receipt page inside the dashboard shell — different from the standalone PWA at `/[portSlug]/scan`. Both upload to the same `/api/v1/expenses/scan-receipt` and `/api/v1/expenses` endpoints, but the user-facing flows diverge (the dashboard one has both camera + file picker buttons; the PWA one is camera-first). Confusion risk; pick one or clearly label the dashboard surface as "Upload receipt (desktop)" vs the PWA "Scan receipt".
### M11. `interests/interest-list.tsx` FAB safe-area offset is hand-rolled
Line 350 hardcodes `bottom-[calc(env(safe-area-inset-bottom)+86px)]` where 86 = tab-bar height (56) + 30px gap. If tab-bar height changes, FAB collides. Extract `MOBILE_TAB_BAR_HEIGHT` to a shared constant or CSS var.
## Quality nits
- Scanner manifest `short_name: 'Scanner'` vs `appleWebApp.title: 'PN Scanner'` → installed-app label differs across iOS/Android. Unify on "PN Scanner".
-`safe-left`/`safe-right` Tailwind utilities are declared (`tailwind.config.ts:150–154`) but never referenced anywhere in `src/`.
-`must-revalidate` on manifest `Cache-Control` is redundant alongside `max-age=300`.
## What's solid
- Per-port dynamic manifest with proper `scope` + `start_url`.
-`viewportFit: 'cover'` + safe-area-inset utilities in topbar/bottom-tabs.
-`-webkit-tap-highlight-color: transparent` global (`globals.css:98`).
| CRIT | Privilege escalation via permission-overrides PUT and role reassignment | `permission-overrides/route.ts`, `users.service.ts updateUser` — refuse to grant any leaf the caller doesn't hold |
| HIGH | `/api/v1/alerts` GET ungated | Add `withPermission('admin','view_audit_log')` or filter payload |
| MED | Residential toggle silently overrides port-role-override on `residential_*` | Document precedence in helpers.ts or compose via deepMerge |
| MED | `withAuth` runs three sequential override queries per request | Parallelize override fetches |
A repo-wide grep for any of those five keys returns only the writer — **no client form / page / hook ever reads `prefill_*`**. So Convert: (a) flushes the inbox row to "converted" eagerly, (b) drops the operator on a blank New Client form, (c) loses the `inquiry_id ↔ client/interest` linkage permanently because nothing persists it. The triage state is now lying ("converted" with no downstream entity), and operators retype the payload from the inbox card. Either consume the params in `client-form.tsx` (with a hidden `inquiry_id` that the create endpoint persists into `clients.metadata` or a new `inquiry_origin_id` FK), or revert the eager state flip so the inbox stays honest until the client is actually saved.
### C2. Two parallel intake pipelines with no correlation → duplicate interests + zombie inbox rows
`/api/public/interests` directly creates `clients + yacht + interest` rows and queues notifications. `/api/public/website-inquiries` is the new "dual-write capture" that stores the raw payload in `website_submissions` for triage. The website is expected to call both (the docstring on `website-inquiries/route.ts` says "AFTER its existing NocoDB write succeeds"). Nothing links them. Result for a single berth-form POST:
1.`interests` row created automatically with notifications fired.
2.`website_submissions` row inserted with `triage_state='open'`.
3. Operator opens the inbox, sees the "open" card, clicks Convert → second `interests` row.
4. Inbox UI never sees that step 1 already happened; the heat-scored interest from step 1 is silently shadowed.
Either the public form path should write `submission_id` onto the created `interests` row (and the inbox should auto-mark `converted` whenever a matching interest exists), or the two pipelines need to be merged into one. Right now they coexist and contradict each other.
`public/interests/route.ts:91` matches `clientContacts.value === data.email` (no `lower()`). The supporting `idx_cc_email` index (`clients.ts:91`) is also raw-value, not `lower(value)`. Two POSTs as `Matt@Example.com` and `matt@example.com` produce **two separate clients, yachts, interests** — and now the recommender has split history for the same human. The companies branch (`route.ts:122`) gets this right (`sql\`lower(${companies.name}) = lower(${data.company.name})\``); the email branch must match: lowercase on insert and on lookup, plus a partial unique index on `lower(value) WHERE channel='email'`.
---
## HIGH
### H1. Residential clients have **no dedup at all**
`residential-inquiries/route.ts:73-93` always inserts a fresh `residential_clients` row. There's no email/phone match, no unique index on `residential_clients.email` (verified — `schema/residential.ts` has no `uniqueIndex`). Every resubmit = new prospect. Sales gets a bloated list of phantoms. Mirror the berth-path dedup (lowercased-email lookup → reuse → open a new `residential_interests` row only).
`inquiry-notifications.service.ts:139-158` reads only `roles.permissions`. The request-time auth path in `lib/api/helpers.ts:227-238` correctly layers role → port-role-override → user-override, but this fan-out helper does not. Symptoms:
- User granted `interests.view` via override only → never gets new-inquiry pings.
- User had `interests.view` in role but their override removed it → still gets pinged.
Either (a) collect every user on the port and run the same `deepMerge` chain per user, or (b) move permission-resolution into a single service helper both callers use.
### H3. Bogus `portId` fails as a 500 (FK violation) instead of 400
`public/interests/route.ts:51-52` accepts the `portId` query/header but never verifies the port exists before the transaction. An invalid id surfaces as a Postgres FK error from `clients.port_id`, returned as a generic 500. The residential endpoint (`residential-inquiries/route.ts:58-61`) validates upfront via `db.query.ports.findFirst` — make the berth route do the same.
### H4. Cross-port email collisions are non-deterministic
`public/interests/route.ts:90-114`: when a client*contact with the same email exists on a \_different* port, the code creates a new client. But `tx.query.clientContacts.findFirst` returns "any matching row" with no `ORDER BY` — subsequent submissions may pick either port's row first. Net: same email used cross-port, then resubmitted to the original port, can spawn 2nd/3rd same-port clients. Fix: filter the lookup by joining to `clients.port_id`, or scope the contact lookup to clients owned by the target port from the start.
The author left a `// future: resolve from getPortBrandingConfig` comment. The moment a second port has a marketing site, every email reads "Port Nimara" regardless of recipient. Wire through `getBrandingShell(portId).portName` (already loaded for the HTML body via `branding-resolver.ts`).
`residential-inquiries/route.ts:150` hardcodes `contactEmail: 'sales@portnimara.com'` in the client confirmation email. The berth path reads the per-port `inquiry_contact_email` setting (`inquiry-notifications.service.ts:44`). Settings UI (`settings-manager.tsx:96`) advertises this setting controls both — but it doesn't. Admins can't reroute residential replies.
### H7. Residential sales alert bypasses the email queue
`residential-inquiries/route.ts:214` calls `await sendEmail(recipients, …)` synchronously inside `sendResidentialNotifications`. The berth path enqueues via BullMQ (`inquiry-notifications.service.ts:51,118`). When SMTP is slow/down, the residential POST hangs (or 500s — though wrapped in `.catch`, the await is fired _after_ the response is returned, so worker eventloop is the only victim) and the notification is lost with no retry. Move to the email queue like the berth path.
`publicInterestSchema` and `publicResidentialInquirySchema` have no `utm_source`, `utm_medium`, `utm_campaign`, `referrer`, `landing_page` fields. `source` is hardcoded `'website'` (`interests.ts:231`). Berth-recommender heat scoring and lead-source dashboards (audit #11) cannot differentiate organic vs paid vs broker referral. The `website_submissions.payload` JSONB at least preserves whatever the website chooses to forward — but `interests` itself stores only the literal string `'website'`. Add an attribution block to both validators + columns (`interests.utm_*`, `residential_interests.utm_*`) and persist what the website hands us.
### M2. Public routes use `req.json(); schema.parse(body)` instead of `parseBody`
`public/interests/route.ts:47-48` and `public/residential-inquiries/route.ts:51-52`. CLAUDE.md explicitly flags this: "Always use `parseBody(req, schema)` from `@/lib/api/route-helpers`" so the error envelope is field-level 400 instead of a generic 500.
`pickName/pickEmail/pickPhone` in `inquiry-inbox.tsx:58-78` use a small set of candidate keys but never compose `first_name + last_name`. Website payloads that send `{first_name, last_name}` without a `name`/`fullName` field render as `(no name supplied)`. Two-line addresses and contact-form payloads silently lose the operator's first hint of who submitted.
`createAuditLog` (`public/interests/route.ts:254-271`) records the interest creation but not whether the client+yacht were created fresh vs reused. Forensics ("did this lead come from the form or get manually entered?") become guesswork. Add `metadata.dedup` = `{ clientReused: boolean, companyReused: boolean }`.
### M7. Yacht inserted with `status: 'active'` even on speculative form leads
`route.ts:179`. There's no "prospect" yacht state, so every unconverted interest still leaves an "active" yacht. Active-yacht counts in reports become inflated. Consider a `'prospect'` status or a deferred-insert pattern keyed on `interests.outcome != 'lost_*'`.
### M8. Admin website-submissions list permission mismatch
The inbox at `/[portSlug]/admin/inquiries` is the marketing-funnel triage surface, but `/api/v1/admin/website-submissions/route.ts:23` gates GET on `admin.view_audit_log`. A sales lead reviewing submissions doesn't conceptually need audit-log access. Introduce a dedicated `inquiries.view` / `inquiries.triage` permission (consistent with the rest of the permission matrix) so this can be granted independently.
---
## Settings application — verified flow
-`inquiry_contact_email` (string, per-port): consumed by berth client-confirmation email (`inquiry-notifications.service.ts:44`); **not** consumed by residential confirmation (H6). Falls back to `sales@portnimara.com` literal.
-`inquiry_notification_recipients` (JSON array, per-port): consumed by berth sales-alert fan-out (`inquiry-notifications.service.ts:106`). Empty array = no external alert. No de-dup against role-based recipients (a user listed here who _also_ has `interests.view` gets two pings).
-`residential_notification_recipients` (JSON array, per-port): consumed by residential alert; falls back to `[inquiry_contact_email]` if empty (`residential-inquiries/route.ts:174-179`). Correct envelope.
Three settings are surfaced on the admin Settings page (`settings-manager.tsx:96-117`) so admins can edit them; default values match the service-side fallbacks.
_This is a forward-looking proposal report, not a defect audit. Grouped HIGH-VALUE / MEDIUM / EXPLORE with effort estimates and "what NOT to AI-ify" critical pass._
# Audit #32 — Improvements, Nice-to-Haves & AI Opportunities
**Scope:** Forward-looking proposals, not a defect audit. Every proposal grounded in real surfaces seen in this repo (file paths cited). For each: user benefit, implementation sketch, effort estimate (S/M/L), and risk note where it matters.
**Effort key:** S = ≤½ day, M = 1–3 days, L = >3 days / cross-cutting.
---
## Section A — UX / Feature Improvements
### A · HIGH-VALUE
**A1. Bulk actions on Berths, Companies, Yachts**
Bulk archive/tag/move flow exists in `src/components/clients/client-list.tsx` + `src/components/interests/interest-list.tsx` (single `/bulk` endpoint per domain), but Berths, Companies, and Yachts use the same `data-table.tsx` shell with `BulkAction[]` support and never pass any. Reps regularly need to retag a batch of yachts after import or move 30 berths to a new pricing band.
- _Sketch:_ Add `bulkActions=[...]` wired through the existing `data-table.tsx` API; mirror the `/api/v1/clients/bulk` and `/api/v1/interests/bulk` endpoint pattern for `berths`, `companies`, `yachts`. `interest-list.tsx` lines 124–280 are the reference implementation.
- _Effort:_ M
- _Risk:_ low — pattern already tested for two domains; ensure permission gate per action mirrors single-entity gates.
**A2. Smart undo banner for archive / outcome / stage-change**
Already have `client-restore.service.ts` + a smart-restore-dialog component, and stage rollback would be supported by audit logs. Reps lose minutes every time they fat-finger an archive or set an outcome on the wrong card on the pipeline board.
- _Sketch:_ After any archive / outcome-set / `interest_archived` / `interest_completed` trigger, raise a Sonner toast with an "Undo" action for 8s, calling the existing restore service or a tiny reverse-mutation endpoint. Hook into the mutation `onSuccess` in `interest-list.tsx`, `client-list.tsx`, `pipeline-board.tsx`, and `interest-outcome-dialog.tsx`.
- _Effort:_ M
- _Risk:_ berth-rules-engine has already fired side-effects (`berth_unlinked`, `interest_completed` cascade). Undo must replay the reverse rule or explicitly skip rule-engine via a `skipRules` flag — otherwise undo leaves stale berth status.
**A3. "What changed since I last looked" digest on detail pages**
The `entity-activity.service.ts` + `use-track-entity-view.ts` infrastructure is already in place — every detail view is tracked. Reps open a deal they haven't touched in a week and have to manually scroll the activity feed.
- _Sketch:_ On detail page load, query activity items with `createdAt > lastViewedAt` (from `recently-viewed.service.ts`) and render a dismissable "3 new things since 5 days ago: signed EOI, +€2k deposit, new note from María" strip above `entity-activity-feed.tsx`.
**A4. j/k row navigation + `o` open + `e` edit + `/` focus filter on list pages**
Cmd-K is already wired in `command-search.tsx`; reps still mouse-hop between rows in `data-table.tsx`. Power users on busy pipeline days are the loudest beneficiaries.
- _Sketch:_ Add a `useListKeyboardNav(rows, activeIndex)` hook used inside `data-table.tsx`. `j/k` move active row, `o`/Enter opens detail, `e` triggers inline-edit on the first inline-editable cell, `/` focuses the filter input. Respect `e.target` being an input.
- _Effort:_ S
- _Risk:_ must be globally disabled inside dialogs/forms — use the same `document.activeElement instanceof HTMLInputElement` guard already in command-search.
Command-search currently navigates but doesn't create. Reps regularly want to drop a client/interest/reminder without leaving the current page (e.g. a quick call comes in while reviewing a berth).
- _Sketch:_ Extend `command-search.tsx` palette with `+ New client`, `+ New interest`, `+ New reminder`, `+ Log call`. Each opens a drawer-mounted minimal form (3 fields max) using the existing forms wrapped in `Drawer` instead of `Dialog`. Re-use `client-form.tsx`, `reminder-form.tsx` in a "compact" mode prop.
Today `client-form.tsx`, `interest-form.tsx`, `expense-form-dialog.tsx`, and `reminder-form.tsx` reset every field. A rep doing 12 interests in a row re-types the same source / currency / lead source.
- _Sketch:_ Persist last-submitted values per form per user under `user_profiles.preferences.formDefaults` (same shape used for `dashboardWidgets` per `widget-registry.tsx` comments). On form open, prefill from preferences, mark prefilled fields with subtle "(last used)" hint. Provide a "Reset defaults" link in the form footer.
- _Effort:_ S
- _Risk:_ leaks tag/source preference into the wrong port for super-admins switching ports — scope key by `(userId, portId, formName)`.
- _Sketch:_ Add `@dnd-kit/sortable` (already in tree if not, very light add). Wire `onDragEnd` to `inline-stage-picker.tsx`'s mutation. Dropping into `won/lost` columns opens `interest-outcome-dialog.tsx` instead of silent set.
- _Effort:_ M
- _Risk:_ berth-rules-engine fires on `eoi_sent` / `contract_signed` triggers — make sure stage drag uses the same `advanceStageIfBehind` codepath, not a raw stage update.
- _Sketch:_ Add `visibility: 'private' | 'shared'` column to `savedViews`; service `list()` returns own + shared. Permission gate: `savedViews.share` (new). Show a "Share" toggle in `save-view-dialog.tsx`.
- _Effort:_ M
- _Risk:_ low — additive; ensure shared views can't expose entity rows the viewer lacks permission for (filter happens server-side on data fetch, not view definition, so already safe).
- _Sketch:_ Add row-checkboxes to `document-list.tsx`, surface `Move to folder` as a bulk action. Reuse existing `move-to-folder-dialog.tsx` accepting an array. Service already supports the operation per-item; wrap in a single transaction.
- _Effort:_ S
- _Risk:_ system-managed folders already reject mutations via `assertNotSystemManaged` — bulk move must respect this per-item and report per-item errors (partial success).
- _Sketch:_ Add quick-buttons row to snooze dialog. Same options as Gmail's snooze. Pre-compute target dates relative to user timezone (already wired via `inline-timezone-field.tsx`).
- _Effort:_ S
- _Risk:_ DST — use the existing `formatInTimezone` helpers, don't add raw ms.
**A11. Dashboard widget: "My open EOIs — needs nudge"**
13 widgets in `widget-registry.tsx`; none surface "EOIs sent ≥ 5 days ago, not yet signed, no reminder set". This is the single most actionable rep widget — the deal that's slipping.
- _Sketch:_ Add a `watchedBerths` array under user preferences, "watch" toggle in `berth-detail-header.tsx`, widget rendering status changes since last view.
`pipeline-card.tsx` is read-only; double-click → edit value/notes in place, mirroring the `<InlineEditableField>` pattern already used everywhere on detail pages.
**A15. "Open in new tab" cmd-click on any entity row**
`data-table.tsx` row click navigates. Need to make every row a real `<a href>` so cmd-click + middle-click behave natively. Power users coming from Linear / Notion will expect this.
- _Sketch:_ Tiny "Saved · just now" timestamp ghost-text near the field for 2s after mutation success; "Saving…" spinner while pending. Surface in `inline-editable-field.tsx` and `inline-tag-editor.tsx`.
`empty-state.tsx` exists but several lists fall back to "No results" plain text (e.g. interest-eoi-tab when no EOI yet, client-yachts-tab when none linked).
- _Sketch:_ Add tiny "copy" icon-button next to `inline-phone-field.tsx`, mooring number display in `berth-detail-header.tsx`, and bank details in invoice detail. Use the standard `navigator.clipboard.writeText` with a 1s "Copied" tooltip.
**B4. Visual indicator for system-managed folders**
CLAUDE.md says folder-tree-sidebar shows lock markers on system folders. Add the same visual rule to `move-to-folder-dialog.tsx` — today the dialog lets you select system folders (and gets rejected later by `assertNotSystemManaged`).
`inline-phone-field.tsx` renders text. Wrap in `tel:` and append a WhatsApp icon linking to `https://wa.me/<E.164>`. For a port-side sales team WhatsApp is the primary channel.
**B7. Toast deduplication for realtime invalidation**
`realtime-toasts.tsx` (touched in current branch). Multi-edit sessions where one rep edits 8 fields generate 8 toasts on the watching rep's screen. Coalesce within 2s.
"send EOI to last-viewed client", "create reminder in 3 days for current client", etc. Recorded by holding a key while performing actions, then invokable via cmd-K → "Run macro".
`timezone-drift-banner.tsx` warns of drift. Extend: every `formatDate` in detail headers shows `Mon 14 May · 14:32 (your time) · 15:32 (client time)` on hover when client timezone is known.
Existing AI surfaces grounded in this repo: `admin/ai` and `admin/ocr` admin pages; `email-draft.service.ts` (compose suggestion via `/api/v1/ai/email-draft`); `interest-scoring.service.ts` (pure SQL — _not_ AI today, candidate for AI uplift); `berth-pdf-parser.ts` (AI is the 3rd parser tier); `expense-ocr.service.ts` + `receipt-scanner.ts` (OCR + structuring); `ai-budget.service.ts` (cost-budget gate). The OpenAI SDK is wired but optional. All proposals below assume model calls go through a service that respects `ai-budget` and an explicit per-port enable flag.
**C1. Auto-summarize a client / interest on detail open**
When a rep opens a client/interest, summarize: "5 EOIs over 18 months, 2 archived, last touched 12 days ago by María, current stage is contract-out — last note suggests cash-flow concern; berth A4 is the primary." Plays directly into A3 (what-changed digest).
- _Sketch:_ New `/api/v1/ai/entity-summary` endpoint accepting `entityType + entityId`, gathering activity log + notes + linked entities (already available via `entity-activity.service.ts`), prompting GPT for a 3-sentence summary. Cache by `(entityId, last_activity_id)` in Redis. Surface as a collapsible card above `detail-header-strip.tsx`. Always show "View source" → activity feed; never hide raw data.
- _Effort:_ M
- _Risk:_ confabulation — model invents a number. Mitigate: structured prompt that returns JSON with `claims: [{text, sourceActivityIds: []}]`, render only claims with non-empty source IDs. Hard 200-token cap.
**C2. Semantic search across notes, email bodies & document content**
`search-nav-catalog.ts` is keyword-based. Reps searching "the client who was worried about wave exposure" can't find anything. The biggest practical AI win in a CRM.
- _Sketch:_ Add an `embeddings` table (pgvector — already supported by Postgres). Embed `notes.body`, `email_messages.text`, signed-document OCR text, on insert via a new BullMQ `embeddings` worker (sibling to `workers/ai.ts`). Add `/api/v1/search/semantic` returning ranked entityIds. Toggle in cmd-K palette between "Exact match" and "Semantic". Cite source row per hit.
- _Effort:_ L
- _Risk:_ PII flowing to OpenAI embeddings. Use a local embedding model (gte-small via fastembed/onnx) per `lib/ai` design — never ship raw notes to OpenAI for embedding. Document this clearly in CLAUDE.md.
`interest-scoring.service.ts` is pure rule-based (pipelineAge, stageSpeed, etc.). It works but reps disagree on signal weights. Train a per-port logistic regression on historical `outcome` (won/lost) using current factors + a few new ones (days since last note, last email response time, deposit pattern). Output a calibrated probability.
- _Sketch:_ New nightly job `train-interest-model` in `workers/ai.ts` using a tiny library (no GPT — pure numerical). Persist coefficients in `system_settings.interest_model`. Service applies them at scoring time. Expose model AUC on `admin/ai`.
- _Effort:_ L
- _Risk:_ per-port data thin (cold start). Default to SQL weights until ≥30 closed interests exist. Document drift detection — refuse to serve a model with AUC ≤ 0.6.
**C4. Smart reminder suggestions from email content**
Inbox (`email-threads-list.tsx`) already exists. When a client email contains "Let's chat next Tuesday" or "I'll get back to you in two weeks", surface a one-click "Create reminder for 21 May".
- _Sketch:_ On new `email_messages` insert, the existing worker calls a new `extractActionableDates(body)` GPT prompt returning JSON `{candidates: [{date, summary, confidence}]}`. Surface as a banner in `email-threads-list.tsx` and in the matching interest's reminder rail. **Never auto-create** — always suggest.
- _Effort:_ M
- _Risk:_ dates in client signatures / disclaimers ("This email was generated on …") fool the model. Filter low-confidence; cap one suggestion per message.
**C5. "Why this berth?" + "Why not?" explanation for the recommender**
`berth-recommender.service.ts` outputs a tier (A/B/C/D) + heat score. Reps can't always articulate to the client why a specific berth made the shortlist.
- _Sketch:_ Add an LLM rephrasing step over the structured tier-reasoning JSON (already produced by the service). Returns plain-English: "Tier A: matches your yacht's 22m LOA + 5m beam, on the protected pontoon, currently available, no historical pushback." Render inside `berth-recommender-panel.tsx`. Source data is fully structured → low confabulation risk.
- _Effort:_ S
- _Risk:_ explanation must never contradict the structured tier. Add an automated unit assertion that the explanation contains the tier label and the dimensions field.
**C6. Auto-draft post-meeting note from a voice memo**
Reps walk back from a viewing with a 90s phone recording. Today they re-type. Drop the audio into the client's notes tab, Whisper transcribes + GPT summarizes into note-friendly bullet points.
- _Sketch:_ Add `audio-note-upload` action to `notes-list.tsx`. Worker pipeline: upload via storage backend → Whisper → GPT bullets → insert as a draft note flagged `ai_generated=true`. Rep reviews + saves.
- _Effort:_ M
- _Risk:_ Whisper accent accuracy on Polish / Italian names. Always preserve the raw audio + transcript alongside the bullets; never delete the source.
- _Sketch:_ Add a translate-icon button to `compose-dialog.tsx` and `notes-list.tsx`. One-click translates a draft into the client's preferred language (already tracked on `clients.preferredLanguage`). Show both versions side-by-side before send.
- _Effort:_ S
- _Risk:_ never auto-translate without rep confirmation, especially for any contractual phrasing.
**C8. Document-template merge-field auto-population from client context**
`merge-fields.ts` catalog + `eoi-context.ts` already do structured population. Where merge fields lack a structured source (admin templates with `{{custom_intro}}` blanks), an LLM could draft from notes + client profile. Rep then reviews.
- _Sketch:_ New "Suggest draft" button on each blank merge field at template-fill time. Returns 2–3 phrasings; rep picks one.
- _Effort:_ M
- _Risk:_ see "what NOT to AI-ify" below — this is borderline. Allowable only for non-legal merge fields (greeting, intro paragraph), explicitly blocked for legal/financial blanks.
**C9. Photo categorisation for berth/yacht uploads**
Berth PDFs are parsed; raw photos uploaded to yacht/berth detail aren't tagged. AI auto-tagging would speed search for "yachts with a bowsprit" or "berths with a fixed davit".
- _Sketch:_ On image upload via `image-cropper-dialog.tsx`'s completion, queue a vision job that returns 3–5 tags (drawn from a controlled vocabulary). Store as photo metadata. Search filters use vocabulary terms.
- _Effort:_ M
- _Risk:_ vision-model bias / hallucinated features. Constrain output to a port-defined vocabulary list; reject anything outside it.
**C10. Conflict / clause-mismatch detection across templates and signed copies**
When admins edit a template, did the new clause contradict something they wrote in another template? When a counterparty returns a "with edits" PDF (currently uploaded via `external-eoi-upload-dialog.tsx`), did they alter a non-trivial clause?
- _Sketch:_ Embed each clause; on template save, surface "this clause is 0.92 similar to but materially differs from a clause in Template X". On external-EOI upload, diff against the canonical template's text and flag deltas in a yellow strip with "Reviewed by [rep]" before the rep can finalize.
- _Effort:_ L
- _Risk:_ false confidence — see "what NOT to AI-ify". Acceptable only as an _assistive flag_, never as a green-light. UI copy must say "Possible material difference detected — review required" not "No material difference".
`expense-dedup.service.ts` handles exact duplicates. Layered AI: detect amounts outside the rolling p95 for the same vendor, or trip-labels that look mismatched against expense date.
## Section C+ — What NOT to AI-ify (critical pass)
These places either carry liability if the model confabulates, or have a tighter ground-truth than AI can match. **Refuse the AI proposal even if it sounds appealing.**
- **Legal text in EOIs, contracts, reservation agreements.** `eoi-context.ts`, `document-templates.service.ts`, `reservation-agreement-context.ts`. The merge-field allow-list (`VALID_MERGE_TOKENS` in `merge-fields.ts`) exists _precisely_ to keep AI out of legal copy. Never AI-generate a clause; never AI-paraphrase a clause "for readability"; never AI-translate a clause and present the translation as binding. Keep all legal text rep-authored or counsel-authored, period.
- **Money flow.** Invoice amounts, deposit allocation, currency conversions, FX rate selection (`currency.ts`, `invoices.ts`). The audit-26 multi-currency audit is in flight precisely because money math has to be deterministic and reconcilable. AI here = unrecoverable customer trust damage on a single mistake.
- **Regulatory / GDPR responses.** `gdpr-export.service.ts`, `gdpr-bundle-builder.ts`. Subject-access requests must return _exactly_ what's in the database, with no LLM summarization layer that could omit a record.
- **Signing decisions.** The Documenso webhook (`handleDocumentCompleted` idempotency, audit-tier 1) is the source of truth that a contract was signed. AI must never infer signing state from email content. If the contract isn't in the webhook stream as `DOCUMENT_COMPLETED`, it isn't signed.
- **Berth assignment auto-commit.** `berth-recommender.service.ts` is intentionally pure SQL; the rules engine is intentionally `suggest` by default. Don't change that — auto-binding a berth to a client based on an LLM "judgment" is exactly the kind of mistake that ends in a refund and an apology. Recommend, never auto-assign.
- **Mooring-number / dimensions parsing.** The 3-tier PDF parser (AcroForm → OCR → AI) escalates to AI only when OCR confidence is low _and_ a rep clicks "AI parse" _and_ a mooring-mismatch confirmation is required at apply time (`berth-pdf.service.ts`). Don't lower any of those guards.
- **Pipeline outcome ("won" / "lost").** This drives revenue reporting (`reports.service.ts`). Setting an outcome must remain a human decision. AI may suggest "this looks won based on the signed contract", but the human clicks the button.
- **Email send-side text in template-driven send-outs.** `document-sends.service.ts` rate-limits and audits. AI-generated wording is fine for free-form composes (`compose-dialog.tsx`) where the rep reviews. AI-generated wording is _not_ fine on bulk template sends where one bad phrasing reaches 50 clients before anyone notices.
- **Audit log entries.** Audit logs (`audit.service.ts`) must remain raw structured events. Never let AI rewrite or compress them.
- **Permission overrides.** `user_permission_overrides` (new in this branch). AI must never suggest or auto-apply grant/revoke — that's a security primitive.
---
## Implementation sequencing recommendation
If the team wants a 2-sprint shipping bundle aligned with the existing branch's themes:
1.**Sprint 1 (UX, low risk):** A1, A4, A5, A6, A11, B1, B2, B3, B5 — everything tagged S or low-M, no new infra.
2.**Sprint 2 (AI runway):** Build the `lib/ai` skeleton (budget gate is in place; need a local-embedding pipeline + a worker) → land C1 (entity summary) and C5 (recommender explanation), both low-risk because they wrap structured data. Defer C2 (semantic search) until the embedding worker is proven.
Every C-section proposal should ship behind a per-port admin toggle (`system_settings.ai_features.<name>`) and respect `ai-budget.service.ts`. Every AI surface must cite its source rows or be flagged as "AI assistance".
— End of report —
---
## 22. Date/time + DST + scheduled jobs audit (datetime-auditor)
# Date/time + DST + scheduled jobs audit — 2026-05-12
`src/app/api/v1/me/avatar/route.ts` POST uploads a NEW file via `uploadFile()` and overwrites `user_profiles.avatar_file_id` — but never reads or deletes the previous id. Every "Replace photo" leaks one DB row + one blob, untethered (no `client_id`/`yacht_id`/`company_id`), so invisible to every existing UI sweep.
.set({ avatarFileId: record.id, updatedAt: new Date() })
.where(eq(userProfiles.userId, ctx.userId));
```
**Fix:** SELECT the prior `avatar_file_id`, call `deleteFile()` (already handles ref-check + blob + audit), wrapped in try/catch so a stale-blob failure doesn't block the new avatar.
---
## HIGH
### H1. `handleDocumentCompleted` put-before-insert leaks signed-PDF blobs on retry storms
`src/lib/services/documents.service.ts:1131-1188`. Sequence: `storage.put` → `db.insert(files)` → `db.update(documents).set(signedFileId)`. The idempotency gate at line 1110 stops a _second_ webhook from minting a _second_ blob — but only if `doc.status === 'completed'` AND `signedFileId` is set, which requires step 3 to have run. If step 2 OR step 3 throws on attempt N, the blob from step 1 survives with no DB pointer; Documenso retries; the gate doesn't trip (status still not 'completed'); step 1 runs again with a fresh UUID storage path. Each retry compounds an orphan.
**Fix:** either insert the `files` row in a `pending` state BEFORE `storage.put` (so failure rolls back via FK / explicit cleanup), or reuse a stable storage key derived from `documents.id` so retries overwrite the same blob.
`src/lib/services/documents.service.ts:596-616` does `db.delete(documents)` only. Both file FKs are plain `references()` (no cascade, no SET NULL) — the document row vanishes but the `files` rows + blobs survive with no link back. For a `cancelled`/`expired` doc with `signedFileId` (the `sent`/`partially_signed` block at line 599 doesn't cover these), the signed contract PDF — containing PII — is permanently orphaned in storage.
**Fix:** in `deleteDocument`, also delete dependent `files` rows via `deleteFile()`, or refuse the delete if files attached (mirroring `deleteFile`'s ref-check).
### H3. Brochure versions: zero cleanup, ever
`src/lib/services/brochures.service.ts:191``archiveBrochure` only flips `archivedAt` + clears `isDefault`. No version-row delete, no blob delete. No "delete prior version" admin endpoint, no retention cron, no rolling cap. CLAUDE.md says "Archived brochures retain version history" — that's by design, but there's also zero path to ever drop one. With ~10 MB PDFs iterated monthly, linear unbounded growth.
**Fix:** admin `deleteBrochureVersion(brochureId, versionId)` endpoint (blob delete via `getStorageBackend().delete()` + row delete in tx); refuse to delete the only remaining non-archived version. Optionally `brochure_version_retention_count` system setting.
### H4. `berth_pdf_versions` has no cleanup mechanism
Symmetric problem. `src/lib/services/berth-pdf.service.ts` inserts a fresh row + UUID-keyed blob per upload (line 213); old versions accumulate forever. `current_pdf_version_id` advances; history-by-design is unbounded-by-default. For a port with hundreds of berths reuploaded under parser iterations, this is the largest storage footprint in the system.
**Fix:** admin "Delete this version" action on the version-history list, gated so the `current_pdf_version_id` cannot be deleted. Storage delete + row delete in a tx.
---
## MEDIUM
### M1. `files.client_id` lacks an explicit `onDelete` — fragile
`src/lib/db/schema/documents.ts:30`: `clientId: text('client_id').references(() => clients.id)` (no `onDelete`). Migration 0000 records `ON DELETE no action`. The only existing client-delete path (`client-hard-delete.service.ts:193`) explicitly nullifies `files.client_id` first, so it works — but any future bulk-delete / port-teardown / dev script bypassing `hardDeleteClient` will FK-violate. Compare `files.yacht_id` + `files.company_id`, both `set null` (added in 0042).
**Fix:** new migration to `ON DELETE SET NULL``files.client_id`. Removes the implicit invariant that hard-delete is the only legal path.
### M2. `demoteSystemFolderOnEntityDelete` is wired for clients only
One caller (`client-hard-delete.service.ts:236`). No hardDeleteYacht / hardDeleteCompany exists today, so not currently broken — but it's a landmine when those flows ship. Both must call `demoteSystemFolderOnEntityDelete(portId, 'yacht'|'company', id)`.
### M3. Hard-deleted-client files become un-swept root orphans
`client-hard-delete.service.ts:193` nullifies `files.clientId` and demotes the system folder to `"{name} (deleted)"`. The file rows now have `clientId=null` + `folder_id` pointing at the demoted folder — discoverable in the demoted folder but never automatically dropped. The HARD delete of the client doesn't actually hard-delete their files. Inconsistent with the "hard" naming AND with GDPR Article 17.
**Fix:** mid-transaction (before the nullify), capture the affected file IDs; post-transaction call `deleteFile()` on each (handles blob + audit). Alternatively: nightly worker that drops file rows where every entity FK is null + no doc/expense/maint reference + `created_at < N days`.
### M4. GDPR export cleanup retries forever on storage failure
`src/lib/queue/workers/maintenance.ts:97-108`. If `storage.delete(row.storageKey)` throws, the catch increments `failed` but does NOT delete the DB row. Next 4 AM run, same row reappears; same failure; same warn. No max-retry, no dead-letter, no admin escalation. A permanently broken storage path silently piles up infinite warns AND the GDPR-erasure obligation never completes.
**Fix:** track `delete_attempts` per row; after N failures either force-delete the DB row + log the orphan-blob to an admin-visible orphans table, or escalate at pino `error` + Sentry.
### M5. `migrate.ts` table list has no drift guard
`src/lib/storage/migrate.ts:52` explicitly admits: _"The `report_snapshots` table called out in the audit does not exist yet. Add it here when it lands."_ This is a manual checklist with no enforcement — any future table that adds a `storage_key`/`storage_path` and forgets to extend `TABLES_WITH_STORAGE_KEYS` will silently leave its blobs behind on every backend swap.
**Fix:** integration test that diffs `information_schema.columns WHERE column_name IN ('storage_key','storage_path')` against `TABLES_WITH_STORAGE_KEYS`. Failing test forces an update before the new table can ship.
### M6. `deleteFolderSoftRescue`: no per-row audit + opaque sibling-name collision
- Only the folder delete itself is audit-logged; the bulk re-parent of N documents + N files leaves no per-row trail. An auditor cannot reconstruct "which folder did this signed contract land in?"
- If a re-parented child folder's name collides with an existing sibling at the destination, the UPDATE throws on `uniq_document_folders_sibling_name` and the tx rolls back. Error propagates as a raw "duplicate key" — compare `moveFolder`, which catches via `isSiblingNameConflict` and returns a useful 409.
**Fix:** (a) emit one bulk audit row with `metadata: { docsMoved, filesMoved, rescuedTo }`; (b) wrap the UPDATE in the same conflict catch.
`document-folders.service.ts:95` logs `"listTree: orphan folder row … dropped from tree"`. Defensive — but the orphans aren't auto-healed and aren't surfaced anywhere. Post-soft-rescue this shouldn't happen, but if it does (race, manual SQL, future bug), the row hides forever.
**Fix:** daily maintenance worker counts `documentFolders WHERE parent_id IS NOT NULL AND parent_id NOT IN (SELECT id FROM documentFolders)` and emits a metric / log.
`PortProvider` reads the URL slug at render and reconciles Zustand inside a `useEffect`. `apiFetch` reads `useUIStore.getState().currentPortId` synchronously. For a super-admin who is on `/port-A/clients` and clicks `/port-B/clients` (or hits a deep link from search/external nav), the first round of queries fires **before** the reconcile effect commits — sending `X-Port-Id = port-A` while the page chrome renders port-B. Listings come back from port-A and render inside port-B's shell ⇒ **silent cross-port data bleed** in the UI.
`handlePortChange` does invalidate React Query AND push the route, but `setPort` (Zustand setter) is sync — and the `router.push` is async. Any queries kicked off by the new route's components before the next tick can still read stale state on the initial mount. The reconcile happens on the second render.
**Fix sketch:** Have `apiFetch` derive `portId` from `window.location.pathname` FIRST and fall back to Zustand, not the reverse. The slug is authoritative; Zustand is a cache. (The current code only consults the URL when Zustand is empty.)
### C2. `apiFetch` slug-to-id fallback is dead for non-super-admins
The fallback for "Zustand not hydrated yet" calls `/api/v1/admin/ports`. That endpoint has `requireSuperAdmin(ctx, 'admin.ports.list')` (`src/app/api/v1/admin/ports/route.ts:16`). For a port director on a hard refresh, the request 403s, `resolvePortIdFromSlug` returns `null`, `apiFetch` ships the request with **no X-Port-Id header** — and `withAuth` then falls back to `preferences.defaultPortId`, which (per next finding) is also unwritable. End state for the user: a 400 "Port context required" on every initial request after a cold reload, until Zustand re-hydrates from localStorage. Suggest a public/authed `/api/v1/me/ports` lookup that is permission-free.
### C3. `defaultPortId` preference is read by `withAuth` but the `/me` PATCH allow-list refuses to write it
`src/lib/api/helpers.ts:160-164` reads `(profile.preferences as { defaultPortId?: string })?.defaultPortId` as the X-Port-Id fallback.
`src/app/api/v1/me/route.ts:45-66` defines `preferences` with `z.object({...}).strict()` and the allow-list `ALLOWED_PREF_KEYS = new Set(['dark_mode', 'locale', 'timezone', 'tablePreferences'])` at line 154. `defaultPortId` is silently stripped at every write. The fallback in `withAuth` is therefore dead — `preferences.defaultPortId` can only ever be set by a hand-rolled `db.update`. For super-admins this means: no header ⇒ no portId ⇒ `ctx.portId = ''` ⇒ every WHERE `port_id = ''` returns empty. Mild UX bug for super-admins but **silent**. Either remove the dead fallback or add `defaultPortId` to the strict schema + allow-list.
---
## HIGH
### H1. `searchOtherPorts` ignores per-port ACL for super-admin extension (theoretical, currently fine)
`src/lib/services/search.service.ts:1232-1314`. The docstring at line 31 promises "ports the user can access other than `portId`". The implementation just excludes `excludePortId` and joins every other row in `ports`. Today super-admins can access every port, so the behavior matches. Risk: if a future "regional super-admin" role lands and reuses this code path (`opts.includeOtherPorts && opts.isSuperAdmin`) the leak is total — no ACL filter. Recommend passing in the **set of accessible portIds** as a parameter and using it in the `port_lookup` CTE WHERE, even though the current gate is binary.
### H2. `/api/v1/admin/users/[id]/permission-overrides` PUT — port directors can promote anyone in their port to "owns everything"
The route gates on `admin.manage_users` (port-scoped), and rejects self-target (line 163) + targets not assigned to the same port (line 173). But there is no guard preventing a port director from writing `admin.permanently_delete_clients: true`, `system_backup: true`, `manage_users: true`, etc. onto a _different_ user in the same port — and then logging in as that user (or asking that user) to act with elevated permissions. Self-target is blocked but **co-conspirator escalation** is not. Mitigation idea: cap the overrides a non-super-admin can set to the leaves they themselves hold (effectively `ctx.permissions ∩ overrides`). The audit log is recorded, so this is detectable post-hoc, but not prevented.
`src/app/(dashboard)/[portSlug]/admin/layout.tsx:31-33` redirects every non-super-admin away from `/[portSlug]/admin/...`. Meanwhile `/api/v1/admin/**` endpoints are mostly gated on `admin.manage_settings` / `admin.manage_users` / `admin.view_audit_log` — leaves that the port-director role holds. So a port director can hit the APIs (via curl, scripts, or non-`/admin` UI surfaces such as `settings/`) but the matching UI is hidden behind a super-admin redirect. Pick a side: either gate the API endpoints on `requireSuperAdmin`, or let port directors into the corresponding sub-pages of `/admin/` (alerting on the ones that should remain super-admin only — backup, queues, storage, ports, invitations).
### H4. Super-admin with empty `ctx.portId` silently filters to zero rows
`src/lib/api/helpers.ts:166-168` — only non-super-admins are blocked when `portId` is null. A super-admin without an X-Port-Id header AND without a preferences.defaultPortId (which is currently every super-admin per C3) gets `ctx.portId = ''`. Downstream services that do `WHERE port_id = ${portId}` silently return empty data, which is harmless. But endpoints that BRANCH on `isSuperAdmin ? undefined : ctx.portId` (e.g. error-events `route.ts:32`) will hand `undefined` to the service and return EVERY tenant's rows. Currently only the error-events listing does this — but the pattern is risky. A scoped super-admin with the wrong header today sees one port; without the header they see ALL ports — surprising to admins debugging "why am I seeing port-X data on port-Y?". Recommend an explicit `?allPorts=true` opt-in on those endpoints rather than coupling cross-port reads to a missing header.
---
## MEDIUM
### M1. Port switcher only invalidates queries, doesn't abort in-flight ones
`src/components/layout/user-menu.tsx:65-73`. `queryClient.invalidateQueries()` marks queries stale but lets in-flight ones finish and write into the cache. If a long-running fetch (e.g. PDF generation, expensive report) was started under port-A and resolves after the user switches to port-B, the cache entry is now port-A data keyed on a query that the new page treats as port-B. Worth pairing with `cancelQueries()` and a re-key on portId (most query keys appear to not embed portId).
### M2. `/api/v1/expenses/export/parent-company` lost its `isSuperAdmin` guard
`src/app/api/v1/expenses/export/parent-company/route.ts:9-12`. The comment says "Hard isSuperAdmin check used to lock out port admins who held expenses.export = true" — but the check is no longer in the route body, it now relies on the perm gate alone. The service `exportParentCompany` is single-port (filters `expenses.port_id`), so this is not a cross-port leak today. But the doc-vs-code drift should be reconciled either by adding `requireSuperAdmin` back or by deleting the stale comment.
### M3. Search "otherPorts" cross-port hits expose port-level metadata to ALL super-admin queries
`src/lib/services/search.service.ts:1862-1864`, `src/app/api/v1/search/route.ts:20`. Toggle `includeOtherPorts` defaults to false — but any super-admin can flip the query param. The merge into `SearchResults.otherPorts` returns `portId/portSlug/portName/type/id/label/sub` from up to 5 other ports per request without rate-limiting the cross-port enumeration. Pairs with the existing search rate-limit (if any) — confirm and add a tighter ceiling on `searchOtherPorts(query, limit)`. Currently `limit` defaults to whatever the searchQuery schema permits.
### M4. Super-admin dashboard redirect always picks first port alphabetically
`src/app/dashboard/page.tsx:24-27` — `db.query.ports.findFirst({ orderBy: portsTable.name })`. Predictable and stable, but ignores any "last-used port" signal. Combined with C3, a super-admin who manually picks port-B then closes the tab returns to port-A on next login. Cosmetic but disorienting. Easiest fix: persist `last_used_port_id` in `userProfiles.preferences` and read it here.
### M5. Webhook + document workers fan out to ALL super-admins for in-app notifications
`src/lib/queue/workers/webhooks.ts:264`, `src/lib/queue/workers/documents.ts:73`. Both fetch every `isSuperAdmin=true AND isActive=true` user to send notifications. Not a security issue; flagging because a future "regional super-admin" rollout will make the broadcast list quietly cross-tenant. Wrap the queries in a `notifySuperAdmins(portId)` helper now so the porting work is one diff later.
### M6. `/admin/ports/[id]` PATCH lets super-admin mutate any port without the rate-limit gate
`src/app/api/v1/admin/ports/[id]/route.ts:34-50` — no `withRateLimit` on a PATCH that touches every port-wide setting (timezone, currency, branding…). Lower priority because callers are short and trusted, but pairs naturally with the audit log.
### M7. AuthContext has no `accessiblePortIds` set
Every cross-port-aware code path re-derives "which ports can this user touch?" from `userPortRoles` or `isSuperAdmin`. Hoist into `AuthContext` (computed once in `withAuth`) so future endpoints don't have to re-implement the resolution and so cross-port filters can apply `inArray(table.portId, ctx.accessiblePortIds)` uniformly.
-`/api/v1/admin/audit` is strictly port-scoped (line 40) — no cross-tenant peek even for super-admins.
-`withAuth` correctly refuses requests where the body tries to set `portId` (header-only); body-based `portId` is documented as forbidden (line 156-159).
- Reports service consistently uses `ctx.portId` in WHERE clauses (`reports.service.ts:103-163`); no super-admin cross-port aggregation paths.
- Public berth/inquiry endpoints take their portId from a query param / dedicated header, never from auth context — correctly decoupled from session port.
- **Routing:** `getStorageBackend()` reads global `system_settings.storage_backend` ('s3'|'filesystem'), caches by config fingerprint, invalidated via `resetStorageBackendCache()` on settings write or migration flip. Code never imports `minio/Client` outside `s3.ts` (verified — only legacy `buildStoragePath` helper survives in `src/lib/minio/index.ts`). Interface methods: put/get/head/delete/listByPrefix/presignUpload/presignDownload — both backends implement all 7.
## CRITICAL
### C1. `backup_jobs.storage_path` missing from `TABLES_WITH_STORAGE_KEYS` — silent backup loss on backend swap
`src/lib/storage/migrate.ts:55-60` lists only `files`, `berth_pdf_versions`, `brochure_versions`, `gdpr_exports`. `backup_jobs.storage_path` (pg_dump artefacts written by `src/lib/services/backup.service.ts:54+72`) is **not** in the list. Flipping S3 → filesystem (or vice-versa) leaves every historical database backup pointing at the old backend — `getBackupDownloadUrl(id)` will 404 / NoSuchKey, and the admin won't know until they try to restore. This is the worst category of data loss because backups are the recovery path of last resort. The comment in `migrate.ts:51` calls out `report_snapshots` as a future addition but mentions nothing about `backup_jobs`. **Add `{ table: 'backup_jobs', keyColumn: 'storage_path', pkColumn: 'id' }` and ship the line with a smoke test.**
### C2. Orphan-blob risk: every `backend.put` runs outside the `db.insert(files)` transaction
Pattern repeated across 9+ services (`files.ts:68-92`, `documents.service.ts:833-854` and `1134-1183`, `external-eoi.service.ts:71-96` — comment at L67-70 explicitly acknowledges "orphan reaper handles those" but **no reaper exists**, `invoices.ts:603`, `document-templates.ts:537,674`, `reports.service.ts:231`, `gdpr-export.service.ts:169`, `backup.service.ts:62`, `berth-pdf.service.ts:229`). Sequence is: PUT bytes → DB INSERT. If insert fails or the process dies in between, the blob is permanent and unreferenced. Only `handleDocumentCompleted` (`documents.service.ts:1110`) has an early-return idempotency gate; the rest leak. Over months of operation an S3 prefix accumulates dozens-to-hundreds of orphans that pay storage cost forever and survive every backup-restore. **Add an orphan-reaper maintenance job** that walks `listByPrefix()` against the union of all `storage_*` columns and deletes blobs older than 24 h without a DB pointer. Also wrap the `put + insert` pairs in a try/catch that explicitly deletes on insert failure (cheap defense in depth).
`S3Backend.put()` (`src/lib/storage/s3.ts:191`) passes only `Content-Type` to `client.putObject`. No `x-amz-server-side-encryption` header. Bytes-at-rest encryption depends entirely on the bucket's default-encryption policy, which is invisible to the application — a customer who provisions a MinIO/B2/R2 bucket without server-side encryption gets cleartext signed contracts, GDPR exports, and `pg_dump` archives sitting on disk. Same audit posture as SMTP/IMAP creds (which **are** AES-GCM in the DB) demands the same guarantee for the blob plane. **Either add `ServerSideEncryption: 'AES256'` to every `putObject` call, or surface a boot-time check that asserts the bucket has default-encryption enabled and refuses to start otherwise** (similar to the `MULTI_NODE_DEPLOYMENT` guard on FilesystemBackend).
## HIGH
### H1. Berth-PDF presigned-upload keys are not port-scoped
`src/app/api/v1/berths/[id]/pdf-upload-url/handlers.ts:58` builds `berths/{berthId}/uploads/{uuid}_{name}` — no leading `${portSlug}/`. Result: the optional port-binding (`p` field on the HMAC token, enforced in `filesystem.ts:184-188` and documented in `index.ts:43-49`) cannot be wired here, and the storage-key namespacing convention diverges from `buildStoragePath` (which always prefixes the port slug). Tenant isolation today relies on the up-front `berths.portId === ctx.portId` check before mint, but the defense-in-depth port-binding is unwired. **Normalise the key to `${portSlug}/berths/...` and pass `portSlug` into `backend.presignUpload`.**
### H2. `presignDownload` callers never pass `portSlug` — port-binding token guard is dead code
`presignDownloadUrl(...)` (`storage/index.ts:233`) accepts `portSlug` and only 1 of ~12 callers uses it. `files.ts:117,128`, `backup.service.ts:115`, `portal.service.ts:351`, `reports.service.ts:170`, `gdpr-export.service.ts:224,282` all pass undefined. The filesystem-proxy GET will therefore accept any valid HMAC token regardless of the storage-key's port prefix. The check is genuinely defensible (see `filesystem.ts:179`) but never engaged. **Plumb the active port slug through every call site, or remove the optional `p` field and the verifier code so the contract isn't misleading.**
### H3. `S3Backend.put` and `backup.service` buffer entire blobs into memory
`s3.ts:187` (`Buffer.isBuffer(body) ? body : await streamToBuffer(body)`) and `backup.service.ts:60-62` (concatenates the entire pg_dump dump into memory before put). For a multi-GB database dump the worker OOMs. Comment at `s3.ts:184-187` explicitly says "typical files are under 50MB" but `runPgDump` writes a dump file whose size scales with the tenant. **Use `client.fPutObject` (file-path streaming) for backups; for streamable callers expose a `putStream(key, stream, sizeBytes, opts)` overload that pipes without `streamToBuffer`.**
### H4. Migrator's `copyAndVerify` double-buffers every blob and has no streaming hash
`storage/migrate.ts:170-204` reads source → Buffer, sha256, put, then re-reads target → Buffer, sha256 again. For a 5 GB pg_dump (see C1 — once added) this allocates ~10 GB of heap. The sha256-verify round-trip is the right idea; **pipe through `crypto.createHash` on both legs**, never buffer.
`s3.ts:249-256` only calls `presignedPutObject(bucket, key, expiry)`. The signed URL does not bind `Content-Type` or `Content-Length` — a browser can PUT 1 GB of arbitrary bytes against an EOI-signed key. Caps and magic-byte checks fire only on the **register** call afterwards (`registerBrochureVersion` and `uploadBerthPdf` HEAD-then-stream-first-5-bytes path). That's sufficient for the two consumers today, but the gate is one-deep — a future caller that forgets to wire a register endpoint exposes raw S3 directly. **Switch to MinIO `presignedPostPolicy` with `content-length-range` + `Content-Type` conditions so the binding is on the signature itself.**
## MEDIUM
### M1. CLAUDE.md drift on "TABLES_WITH_STORAGE_KEYS populated in 9a5ba87"
CLAUDE.md says the migrator covers "every blob in `files`, `berth_pdf_versions`, `brochure_versions`, `gdpr_exports`". Verified true — but **backup_jobs is the missing 5th** (see C1). Update the doc + add a unit test that asserts the array matches the set of tables with a `storage_*` column.
### M2. `email-compose.service.ts:124` reads attachment bytes into a Buffer
Each attachment under the `email_attach_threshold_mb` cap is fetched via `storage.get(...)` and concatenated. With multiple recipients × multiple attachments the worker holds N × size MB simultaneously. **Stream into `nodemailer`'s `content: <Readable>` API directly.**
### M3. UUID storage keys never check existence before put (no `If-None-Match: "*"`)
`crypto.randomUUID()` collision is astronomical, but a buggy caller passing a duplicate key (or a re-run of a worker after a partial DB rollback) silently overwrites. **Cheap belt: pass `If-None-Match: '*'` (S3) or `O_EXCL` (filesystem) — surfaces double-writes loudly.**
### M4. Per-port S3 routing not possible / `listByPrefix` unbounded
Storage config rows are global (`portId IS NULL`). Multi-tenant can't direct port-A vs port-B to separate buckets / KMS keys. `listByPrefix` returns every key in one array — script-only today but a footgun if called with empty prefix in production. **Document the global-config assumption; add a cursor variant before any per-port-bucket customer lands.**
Cache swaps, but tokens minted under the old root still verify HMAC; `resolveKeyForProxy` then 404s under the new root. Customer download links emailed an hour earlier break with no warning. **Either refuse runtime root changes, or warn in admin UI.**
### M6. Avatar URLs re-presign every 15 min — browser cache broken
No CDN / `s-maxage` fronts hot reads. Per-page avatar GET burns a presign + S3 round-trip. **Issue 24 h URLs for `category='avatar'`, or front with the Next.js Image route.**
-`withTimeout(...)` wraps every minio call (s3.ts L143/150/190/203/219/237/285/292/300); `system-monitoring.service.ts:153` adds its own 5 s deadline. **No bare minio calls escape.** ✓
-`MULTI_NODE_DEPLOYMENT` guard reads `env.MULTI_NODE_DEPLOYMENT` (zod-coerced, `env.ts:80`), test at `filesystem-backend.test.ts:139`. ✓
- **S3 direct PUT:** no inline check (relies on the register endpoint). Acceptable per CLAUDE.md, but document divergence: a future S3 consumer that forgets to call register leaks the gate.
## 34. Dependency upgrade analysis — Context7-assisted (follow-up after deps-auditor)
_Post-session follow-up. Where the original `deps-auditor` covered abandonment + vulnerabilities, this section queries upstream changelogs via Context7 to weigh the pros/cons of pulling every available major. Use this as your bump roadmap._
# Dependency upgrade analysis (Context7-assisted)
Companion to the deps-auditor report from the original 33-agent run.
That auditor checked vulnerabilities + abandonment + license risk; this
follow-up adds **per-dep pros/cons of bumping to the latest stable**,
informed by upstream changelogs/docs queried via Context7.
**Top-line baseline:** `pnpm audit` reports **0 known vulnerabilities**.
No GPL/AGPL contamination. Lockfile reproducible. We are safe TODAY
without any upgrade; everything below is "should we pull the next
- **`middleware.ts` is renamed to `proxy.ts`** in Next 16. The named export `middleware` → `proxy`. Config flags rename (`skipMiddlewareUrlNormalize` → `skipProxyUrlNormalize`). **Edge runtime is NOT supported in `proxy`** — if you need edge runtime you must keep `middleware.ts` (we already use the Node runtime, so this is just a rename for us).
- Async `cookies()` / `headers()` / `params` / `searchParams` was the Next-15 change; Next 16 hardens the warning into an error. We're already async-safe (CLAUDE.md confirms the upgrade landed).
- Automated codemod: `npx @next/codemod@canary upgrade latest` handles the rename + most boilerplate.
-`src/middleware.ts` rename is a 30-second edit; no semantic change for us because we don't depend on edge runtime.
- The Documenso webhook + websocket server custom-server path (`src/server.ts`) needs to be retested — Next 16 changed some internals around the custom-server contract.
-`eslint-config-next` must bump in lockstep (already at 15.5.18 → 16.2.6).
- Turbopack defaults shifted; our dev script (`next dev --turbopack -H 0.0.0.0`) needs a quick smoke run.
**Recommended:** **WAIT 2-4 weeks.** Next 16 dropped recently; let the field's bug reports settle. Then run the codemod + a full playwright smoke. Effort: 1-2h.
- Top-level format helpers — `z.email()` / `z.uuid()` / `z.url()` etc. replace `z.string().email()` / `.uuid()` / `.url()`. Old form is **deprecated** but still works.
- Error customization unified: `{ message: '...' }` → `{ error: '...' }`. Old form deprecated.
-`z.function()` API completely redesigned — now takes `input`/`output` schemas upfront, returns a function factory (not a schema).
- ~14× perf improvement on parse paths.
- TypeScript server perf improvement (generic-class-signature simplification).
- We have ~30 validator files using `z.string().email()` / `.uuid()` style and `{ message: '...' }` style throughout. Both still work in 4.x but produce deprecation warnings on every parse — noisy in logs.
-`@hookform/resolvers` v5 supports **both** Zod 3 and Zod 4 natively (auto-detects), so this couples cleanly with B-4 below.
- We don't use `z.function()` anywhere, so the biggest breaking change is a non-issue for us.
**Recommended:** **GO once Tier A is in.** Codemod-friendly: a single Find/Replace pass on `z.string().email()` → `z.email()` etc. covers ~95% of the churn. Effort: 2-3h including running full vitest + writing replacement codemods.
- We have a custom `tailwind.config.ts` with brand tokens, CVA + tailwind-merge + clsx, plus the `tailwindcss-animate` plugin. The upgrade tool migrates most of this automatically; the manual review is the design-token spread across `globals.css`.
- shadcn/ui components (`components/ui/*`) use `cn()` + arbitrary values heavily. Some `[--variable]` syntax has changed in v4.
-`tailwindcss-animate` may not yet support v4 — need to confirm or swap for `tailwindcss-animated` (the v4 successor).
**Recommended:** **HIGH-RISK / HIGH-REWARD.** Park until you have a clear afternoon. The build-time speedup is genuinely meaningful for dev experience. Run the official upgrade tool on a throwaway branch first; visually diff a handful of critical pages before merging. Effort: 3-4h on a focused day; visual regressions are the variable.
- Coupled with the Zod 4 upgrade — if we stay on Zod 3, v5 still works (the resolver detects Zod-3 schemas via shape probing). Bumping resolvers without bumping Zod is safe.
**Recommended:** **GO IN LOCKSTEP with B-2 (Zod 4).** Effort: 5 min once Zod 4 is in.
**Upstream summary:** Library "/gajus/archiver" not found in Context7 — fallback to npm changelog. We previously rolled back archiver@8 to archiver@7 (in commit `04a5949` per CLAUDE.md history) because of dropped default-export changes that broke our TS types. v8 stabilised since then.
- Last time we tried this it broke. Read the v8 changelog before retrying.
- Used only for GDPR export + backup-restore — narrow blast radius. A failed upgrade is non-customer-facing.
**Recommended:** **DEFER.** Stay on 7 until either v8 demonstrably fixes a CVE / bug we care about, or until we have a green test suite to verify nothing regressed. Re-attempt only when there's a forcing function.
- Used in ~6 surfaces (reminder form, EOI date fields, expense date, invoice due-date, dashboard date-range picker). A breaking change to the calendar render path would affect every form.
**Recommended:** **DEFER 2-3 weeks** to let bug reports surface. Effort to actually do it: ~1h once the spec is reviewed.
- **`@radix-ui/*`** — all current. These ship patch updates frequently; consider a quarterly sweep but not blocking.
- **`@dnd-kit/*`, `@pdfme/*`, `socket.io`, `bullmq`, `pino`, `postgres`, `minio`, `ioredis`, `pdf-lib`, `pdfkit`, `sharp`, `tesseract.js`, `recharts`, `cmdk`, `vaul`, `sonner`, `zustand`, `next-themes`, `date-fns`, `clsx`, `class-variance-authority`, `jose`, `nodemailer`, `mailparser`, `imapflow`, `openai`, `lucide-react`, `react-easy-crop`, `react-hook-form`** — all current within their major lines and either no risk-worthy bump available or already bumped.
---
## Recommended sequencing
1.**Now** — pull Tier A patches as one commit (~5 min).
4.**2–4 weeks** — Next 15 → 16 + `eslint-config-next` 16 + `eslint` 10. Lockstep. Run `@next/codemod` first.
5.**When a tester-friendly afternoon opens up** — Tailwind 4 via the official upgrade tool, with visual review across critical pages.
6.**Defer indefinitely** — archiver 8, react-day-picker 10 (neither is delivering us anything we need).
**Non-goal:** chasing the bleeding edge on every dep. The audit's baseline finding stands — we are secure today. These are mostly developer-experience and perf wins, not security blockers.
| **`react-email`** + `@react-email/components` | 8 hand-strung HTML templates in `src/lib/email/templates/` | Each becomes a `.tsx` component, rendered via `await render(<…/>)` then handed to nodemailer unchanged | 2-3h (one template at a time) |
| **`@tanstack/react-virtual`** | Pagination on `client-list`, `yacht-list`, `berth-list`, `audit-log-list`, `inbox` | `useVirtualizer({ count, estimateSize })` inside the list shells | 1h per list × 5 lists |
| **`ts-pattern`** | 19-case dispatch in `search.service.ts`, 13-case Documenso webhook, 12-case `client-restore.service.ts`, 10-case `recently-viewed/route.ts`, 10-case `custom-fields/[entityId]/route.ts` | `match(input).with(...).exhaustive()` | 30 min per site; start with the Documenso webhook |
| **`unpdf`** | Hand-rolled text extraction in `berth-pdf-parser.ts` | `extractText(await getDocumentProxy(buf))` | 1h |
| **`@formkit/auto-animate`** | One-liner `useAutoAnimate()` ref on any list. Drops into: deal pipeline kanban (pipeline-board.tsx), reminders rail, alerts rail, files list, notes list. Zero CSS. ~2kb. | 5 min per site |
| **`motion`** (formerly framer-motion) | Layout animations for kanban reorder (currently snaps), Vaul drawer enter/exit polish, sheet/drawer slides, `<AnimatePresence>` for inline edits. ~15kb gzip but tree-shakes well. | 1-2h to wire the kanban first |
| **`use-debounce`** | Replaces ad-hoc `setTimeout` debounce in `yacht-picker`, `client-picker`, `audit-log-list`, `send-document-dialog`, `custom-fields-section`, `berth-picker`, `interest-picker`, `dedup-suggestion-panel` (8 sites). Typed `useDebouncedCallback`. ~3kb. | 30 min total |
| **`fast-deep-equal`** | Memo comparator for `DataTable` and React Query `select` functions. Drops re-renders when stable references arrive with new identity. ~1kb. | 20 min |
| **`next-safe-action`** | Type-safe server actions with built-in Zod validation, ownership middleware, `useHookFormAction` hook. Each form drops ~30 LOC of `apiFetch + toastError + mutation-hook` plumbing to ~5. Pairs with `useHookFormAction` which already speaks Zod/RHF. | Migrate gradually — use for new forms first, keep API routes for external callers. Couples with Zod 4 (since safe-action v8+ targets Zod 4 best). |
| **`@axe-core/playwright`** | Accessibility audit during smoke tests. The 33-agent audit flagged WCAG gaps; this catches regressions automatically. | ~30 LOC of test setup. Fails CI on new violations. |
| **`@tiptap/core`** + `@tiptap/react` + extension packs | Real rich-text editor for `notes` (clients/interests/yachts/companies all have polymorphic notes). Currently plain text. Sales reps note things like "call after 4pm UTC, prefers WhatsApp" — bold/italic/links/lists/mentions would help. Tiptap's JSON output format is _already_ in our codebase (the bridge layer), so we'd be storing the same shape we already render. | Decision: keep notes plain or upgrade to rich? If yes, ~3h to wire one entity's notes; the others copy the pattern. |
| **`@next/bundle-analyzer`** | Wraps `next.config.ts`. Generates client + server bundle treemaps after every build. Catches when a tiny PR pulls in recharts on a route that shouldn't have it. The 33-agent audit flagged recharts + pdfme as bundle bloat — this is the tooling to keep that honest. | 15 min setup. Run with `ANALYZE=true pnpm build`. |
| **`@sentry/nextjs`** | Error tracking with frontend + backend correlation, release tracking, source maps, performance traces, replay (optional). We have pino logs but no aggregation/alerting/correlation. Important once we have customer-facing users. | Decision: do we want a SaaS dependency? Self-hosted GlitchTip is also an option (Sentry-protocol-compatible). |
| **`@vercel/og`** (or `satori`) | Generate Open Graph images for shared docs/portal links. Currently the portal has no social previews; if a client shares their EOI link in WhatsApp/Email, the preview is blank. ~10 LOC per route. | 1h for portal share routes. |
| **`papaparse`** | CSV import/export. Sales reps frequently ask for "export to Excel." Plays well with our existing TanStack Table data. ~17kb. | 30 min for client/interest list export. |
| **`@formkit/tempo`** OR **date-fns helpers** | We have **44 files** with hand-rolled `new Date().toLocaleString()` / `.toLocaleDateString()`. Centralize via a `formatDate(date, format, timezone)` helper using `date-fns` (already installed) — no new package needed if we use date-fns's `format`, `formatDistance`, `formatRelative` which we already have. **This is a refactor, not an adoption.** | 2-3h sweep |
| `next-pwa` / `@serwist/next` | PWA assets pending (per MEMORY.md). When that lands, **`@serwist/next`** is the modern choice (next-pwa is unmaintained). For now, skip. |
| `next-intl` / `i18next` / `@lingui/core` | No i18n target today. When we localize, **`next-intl`** is the strongest Next.js App Router integration. For now, skip. |
| `@knocklabs/node` + `@knocklabs/react` | Notification center + channel routing + preferences UI. Likely overkill — we have a simple in-app + email notification system that works. Revisit if we add SMS or push. |
| `inngest` / `trigger.dev` | Background jobs with observability. We use BullMQ; revisit only if we need step functions / cross-service workflows. |
| `posthog-js` | Product analytics + feature flags + session recording. We have Umami for web analytics; PostHog adds product-level tracking. Decision pending. |
| `@growthbook/growthbook` | Feature flags only. We don't have any flagged features today. |
| `fuse.js` / `minisearch` | Client-side fuzzy search. Useful for already-loaded list filtering, but TanStack Table's built-in filter is usually enough. |
| `@uppy/core` + `@uppy/dashboard` | Rich file upload UI with resume, chunking. We have basic file inputs (0 patterns found in audit grep) — not currently a pain point. |
| `@tanstack/react-form` | Successor to react-hook-form by same team. RHF is mature, well-known, and we have 8 forms on it. No compelling migration. |
| `@radix-ui/react-icons` | We use `lucide-react` everywhere. Audit grep shows no imports from `@radix-ui/react-icons`. | Drop after grep-confirm. ~30s. |
| `@pdfme/common` + `@pdfme/generator` + `@pdfme/schemas` | Replaced by `@react-pdf/renderer` in Phase 1. | After PDF migration. |
| `tailwindcss-animate` v1.0.7 | Last published 2024, no v4 support. Replace with **`tw-animate-css`** (the v4-native successor shadcn now recommends). | Required if we move to Tailwind 4. |
| `@types/pdfkit` | Tops at v7.0.0. We're on `pdfkit` v0.18 — types are loose but functional. Keep until we migrate expense-pdf to @react-pdf/renderer. | Defer. |
| `pino-pretty` in `dependencies` | Should be `devDependencies` only — ships ~500kb to prod worker images if it leaks into the runtime path. Audit-verify the build doesn't include it; move if it does. | 5 min check. |
---
### 35.D — Surfaced refactor opportunities (no new package required)
These came up while sweeping for package gaps. They're refactor wins, not
### 35.E — Final adoption order (revised, incorporating section 35)
This supersedes section 34's sequencing where they overlap.
1.**Now (one focused day)** — Zod 4 + `@hookform/resolvers` 5 + **`drizzle-zod`**. One PR. Codemod-friendly. Highest correctness payoff.
2.**Independent (any time)** — **`react-email`** migration of one template (`portal-auth.ts` recommended first), then expand. Independent of any version upgrade.
4.**Independent (any time)** — **`ts-pattern`** in the Documenso webhook switch first (the audit's bug-class poster child), then sweep the other 4 sites.
5.**Independent (any time)** — **`@tanstack/react-virtual`** on `client-list` first, copy pattern to 4 other lists.
6.**Independent (any time)** — **`@formkit/auto-animate`** sprinkle. 5-minute wins per site.
