(opus-4.8 high) Pre-Launch System Review

## Description

Findings are grouped by severity. **Critical** = must fix before launch (data breach / cross-tenant compromise / broken deploy pipeline). **High** = fix before or immediately after launch. **Medium/Low** = should be tracked, not launch-blocking.

---

## Critical

### 1. Any logged-in user can read and overwrite every AI/Airtable/Meedan API key for the whole platform
- **Files**: `src/globals/Settings/index.ts:12-15`, `src/globals/Settings/tabs/AITab.ts:487-502`, `AirtableTab.ts:21`, `MeedanTab.ts:21`, `src/globals/Settings/tabs/MaskedApiKeyField.tsx`
- The `settings` global stores `providerCredentials[].apiKey`, `airtableAPIKey`, and `meedanAPIKey` as plain `type: "text"` fields. Access control is `read/update: ({ req }) => Boolean(req.user)` — **any authenticated user**, regardless of role or tenant.
- `MaskedApiKeyField.tsx` only masks the value in the **admin React UI** (`getMaskedApiKeyPreview`); it wraps a plain text field with no `access.read` restriction at the field or collection level. A logged-in user hitting `GET /api/globals/settings` (REST) or the equivalent GraphQL query gets every key back in plaintext — the masking provides zero server-side protection.
- **Failure scenario**: a volunteer/editor account created just to manage one tenant's political-entity pages calls `/api/globals/settings` and exfiltrates the OpenAI/Anthropic/Google/etc. keys and the Airtable/Meedan tokens for the entire platform, or overwrites them (denial of service, billing abuse, data exfiltration via a swapped Airtable key).
- **Fix**: add a field-level `access: { read: () => false }` (or an admin-only role check) on every credential field, and/or move these to server-only env vars entirely (the app already supports env-var fallback per `.env.example`), removing them from the editable/readable Payload schema.

### 2. Multi-tenant isolation is fully disabled — every authenticated user has cross-tenant access
- **Files**: `src/plugins/index.ts:64` (`userHasAccessToAllTenants: () => true`), `src/collections/Users/index.ts` (`fields: []`, no `access`)
- The multi-tenant plugin's own default for `userHasAccessToAllTenants` is `() => false` (verified in `node_modules/@payloadcms/plugin-multi-tenant/dist/index.js`); this app overrides it to unconditionally `true`. That function gates the tenant-scoping access wrapper (`addCollectionAccess`/`withTenantAccess`) applied to `pages`, `site-settings`, `political-entities`, `tenants`, and to user management — with it hardcoded `true`, the tenant constraint is skipped for **every** authenticated user, and access falls back to each collection's own (unrestricted) access function.
- Compounding this: `Users` has **no `roles` field and no `access` block at all** — there is currently no way to distinguish an admin from a low-privilege tenant editor. Payload's own default access when none is defined is `Boolean(user)` (`node_modules/payload/dist/auth/defaultAccess.js`), so every logged-in user can read/create/update/delete every other user account, and (per the above) every tenant's Pages, PoliticalEntities, and per-tenant SiteSettings.
- **Failure scenario**: a Kenya-tenant editor account logs in and reads/edits Uganda's political entities, pages, and site settings, or promotes their own account by editing another user record — there is no server-side check preventing any of this today.
- **Fix**: implement a real role/super-admin model (e.g. a `roles` field on `Users` and a tenant-membership array), make `userHasAccessToAllTenants` check that role instead of returning `true`, and add explicit `access` functions to `Users`, `Pages`, `PoliticalEntities`, and `PromiseStatus` that check both authentication and tenant membership.

### 3. No production deploy pipeline, and the only existing pipeline deploys unconditionally
- **File**: `.github/workflows/deploy-dev.yml` (only Dokku-dev workflow; `.github/workflows/` contains just this and `claude.yml`)
- The workflow runs on every push to `main`: checkout → Docker build → push to Docker Hub → deploy to Dokku, with **no lint, typecheck, or test step** anywhere in the job (`needs:` is unused; there is exactly one job).
- There is no `deploy-prod.yml` or tag/release-gated workflow at all.
- **Failure scenario**: a broken PR merges to `main` and is live on (dev today, prod tomorrow if this file is copied under time pressure) within minutes, with no automated gate having run.
- **Fix**: add a `test` job (lint + `tsc --noEmit` + `pnpm test:int`) that `deploy` depends on via `needs:`, and add a separate, approval-gated production workflow before launch.

### 4. Database migrations are never run automatically
- Verified via `grep` across `Dockerfile`, `docker/entrypoint.sh`, and `.github/workflows/deploy-dev.yml`: `payload migrate` (defined only in `package.json:16`) is never invoked by the container entrypoint or by CI/CD.
- **Failure scenario**: a PR changes a collection schema; the deploy succeeds; the running app now expects a schema the database doesn't have. This is a silent, easy-to-forget manual step at exactly the moment (launch, then ongoing feature work) where it matters most.
- **Fix**: run `pnpm migrate` as a release-phase/pre-boot step in `docker/entrypoint.sh` before the app starts serving traffic.

---

## High

### 5. `DATABASE_URI` has no fail-fast validation (inconsistent with `PAYLOAD_SECRET`)
- **File**: `src/payload.config.ts:32-34` throws a clear error if `PAYLOAD_SECRET` is missing, but line 74 does `url: process.env.DATABASE_URI || ""` with no equivalent guard.
- **Fix**: mirror the `PAYLOAD_SECRET` guard for `DATABASE_URI`.

### 6. No health-check endpoint / container health probe
- No `/api/health` (or similar) route exists under `src/app/`; no `HEALTHCHECK` in `Dockerfile` or `docker-compose.yml`. The entrypoint script does check Tika readiness internally (`docker/entrypoint.sh:70-92`) but doesn't expose that state externally.
- **Fix**: add a cheap `/api/health` route (DB ping) and wire it into a Dokku/Docker health check.

### 7. `AIExtractions.extractions[].uniqueId` is not required, breaking the Meedan upload matching logic
- **File**: `src/collections/AIExtractions/index.ts:102-110`; matched against in `src/tasks/uploadToMeedan.ts` (matches by `uniqueId`).
- An extraction row saved without a `uniqueId` will never match during the upload-status update, leaving it permanently "pending" even after a successful upload.
- **Fix**: mark `uniqueId` `required: true`, or backfill/generate it in a `beforeChange` hook if it's meant to be system-generated.

### 8. No per-run concurrency guard on the AI extraction workflow/cron
- **Files**: `src/workflows/airtableWorkflow.ts`, job queue config in `src/payload.config.ts` (default `PAYLOAD_JOBS_CRON_SCHEDULE="* * * * *"`)
- The exportRows job already got a "stop pileup, make transaction-safe" fix (per recent commit `a0422fc5`); `extractPromises`/`airtableWorkflow` don't appear to have the same guard. If a run takes longer than the 1-minute cron interval, a second run starts against the same unprocessed documents.
- **Failure scenario**: a slow AI provider makes one `extractPromises` run take 90s; the next cron tick starts a second run over the same documents, producing duplicate AI extractions.
- **Fix**: apply the same "already queued/running" check used for `exportRows` to `extractPromises`/`airtableWorkflow`.

### 9. No timeout on AI provider calls, and no document-size cap before chunking
- **File**: `src/tasks/extractPromises.ts` (`generateText()` calls around the chunk-processing loop; 14,000-char chunking with no upstream size limit)
- A hung provider call blocks the task indefinitely (recovered only by the 2-hour stuck-job cleanup); an oversized or malicious document generates an unbounded number of chunks/API calls with no cost ceiling.
- **Fix**: add a request-level timeout on `generateText()` calls, and a hard max-document-size check before chunking begins (reject/flag oversized documents instead of processing them).

### 10. Missing `error.tsx` / `not-found.tsx` at the frontend root
- **Verified**: `find "src/app/(frontend)"` shows only `layout.tsx` and `page.tsx` at the top level — no `error.tsx`, no `not-found.tsx` anywhere under `(frontend)`.
- **Failure scenario**: an unhandled error on any public page (promise detail, entity page) surfaces Next.js's default error UI instead of a branded, graceful page; unmatched routes may not 404 cleanly.
- **Fix**: add `src/app/(frontend)/error.tsx` and `not-found.tsx`.

### 11. Sequential (non-parallel) data fetching on hot public pages
- **File**: `src/app/(frontend)/[entitySlug]/promises/[promiseId]/page.tsx` — `getTenantNavigation()` → `getPoliticalEntityBySlug()` → `getPromiseById()` → `getPromiseUpdateEmbed()` → `getTenantSiteSettings()` → `resolveMedia()` are awaited one after another; several of these don't depend on each other's output and could run concurrently via `Promise.all`.
- **Fix**: parallelize the independent lookups; this directly affects TTFB on the highest-traffic page type at launch.

---

## Medium

### 12. Webhook auth in `meedan-sync` uses non-constant-time comparison
- **File**: `src/app/api/meedan-sync/route.ts:310` — `providedSecret !== configuredSecret`. Contrast with the airtable-upload route, which uses `crypto.timingSafeEqual`.
- **Fix**: use `timingSafeEqual` here too (pad/length-check first to avoid throwing on length mismatch).

### 13. Deleting a `PromiseStatus` or `PoliticalEntity` leaves denormalized export-row data stale
- **File**: `src/collections/PromiseStatus/hooks/index.ts:27-44` queues a re-sync on status delete but doesn't clear export rows that reference the deleted status first, so rows can carry a `statusId` pointing at nothing with a stale `statusLabel`.
- **Fix**: delete/null out the affected export-row fields before (or as part of) queuing the re-sync.

### 14. AI provider credential resolved once per task, not re-checked if Settings change mid-run
- **File**: `src/tasks/extractPromises.ts` — credentials are resolved at task start; if an admin changes the provider/key while a long run is in progress, the task keeps using the stale credential rather than failing fast or picking up the change.
- **Fix**: acceptable to leave as-is if runs are short; otherwise re-resolve per document/batch.

### 15. No retry backoff on AI provider calls
- **File**: `src/tasks/extractPromises.ts` (`maxRetries: 3`/`4` on `generateText()`), no visible backoff configuration.
- **Failure scenario**: a rate-limited or briefly-outaged provider gets hit with immediate retries across many documents/chunks in parallel, amplifying the outage and burning quota/cost.
- **Fix**: confirm the `ai` SDK's built-in retry behavior includes backoff; if not, wrap with exponential backoff.

### 16. No application-level security headers
- **File**: `next.config.mjs` — no `headers()` config for CSP, `X-Frame-Options`, `Referrer-Policy`, etc.
- **Fix**: add a baseline security-headers config (Next.js `headers()` function), at minimum clickjacking (`X-Frame-Options`/`frame-ancestors`) and a reasonable CSP given third-party embeds (share widgets, media).

### 17. Sentry configured at 100% trace sampling with no PII scrubbing
- **Files**: `sentry.server.config.ts`, `sentry.edge.config.ts` — `tracesSampleRate: 1`, `enableLogs: true`, no `beforeSend`. `meedan-sync/route.ts:190` logs `clientIp`/`userAgent`, which could flow into Sentry breadcrumbs unfiltered.
- **Fix**: lower `tracesSampleRate` for production via env, add a `beforeSend` hook to scrub IP/PII fields before events are sent.

### 18. Backfill migration isn't safely re-runnable if interrupted
- **File**: `migrations/20260423_000000_backfill_ai_extraction_export_rows.ts` — deletes existing export rows for a batch, then `insertMany(..., { ordered: false })`. If interrupted between delete and insert, or if insert partially fails, re-running can leave a partial/inconsistent batch.
- **Fix**: wrap the delete+insert per batch in a transaction, or add a dedupe/reconciliation check.

---

## Low

- **Footer logo missing meaningful alt text** — `src/components/Footer/index.tsx:72` (secondary logo `alt=""`); should carry the org/site name for screen readers.
- **Promise status indicator relies on color alone** — `src/components/PromiseStatus/PromiseStatus.tsx`; consider an icon/pattern alongside color for colorblind users.
- **Hardcoded English fallback strings in `Hero`** (`src/components/Hero/index.tsx:396-406`) will show through on the French locale if a block's config omits copy — move defaults into locale-aware config.
- **ESLint: 0 errors, 39 warnings** (mostly `no-explicit-any` / unused vars), concentrated in `src/tasks/createPoliticalEntity.ts` and `src/tasks/downloadDocuments.ts` — non-blocking cleanup.
- **No dead-letter/failure escalation for the job queue** — failed jobs (`hasError=true`) aren't surfaced to Sentry; an operator has to check the Payload admin Jobs list manually to notice a stuck sync.
- **Stuck-job cleanup at a fixed 2-hour threshold** (`src/tasks/cleanupFailedJobs.ts`) could delete a legitimately long-running large-document extraction rather than just abandoned jobs; consider a longer threshold or archiving instead of deleting.

---

## Verified test/lint status (as of this review)

- `pnpm exec vitest run` (int suite): **7 passed, 1 failed** — the 1 failure (`tests/int/api.int.spec.ts`) requires a live MongoDB Atlas connection unreachable from this sandbox; not a code defect. 32 tests passed, 1 skipped.
- `pnpm lint`: **0 errors, 39 warnings**.
- Dockerfile: multi-stage build, runs as non-root (`nextjs`, uid 1001), uses BuildKit `--mount=type=secret` correctly for `database_uri`/`payload_secret`/Sentry tokens (not baked into image layers), `.dockerignore` excludes `.git`/`node_modules`/`.env*`/tests — this part of the release story is solid.

---

## Checked and ruled out (do not re-flag)

- **`generateMetadata` "missing await" on SEO calls** — an initial pass flagged `return buildSeoMetadata(...)` (no `await`) inside `generateMetadata` in the promise-detail and catch-all pages as breaking SEO metadata. This is **not a bug**: `generateMetadata` is itself `async`, and `return ` inside an async function is automatically flattened/awaited by the JS runtime before the function's own promise resolves — behaviorally identical to `return await `. No fix needed.
- **Public `read: () => true` on `Documents`, `Promises`, `Media`, `AIExtractions`, `Partners`** — flagged by an initial pass as "unauthenticated data exposure." Given PromiseTracker's purpose (public accountability/transparency of promises and their evidence), public read access to this content is very likely intentional product behavior, not a defect. Worth a one-line confirmation with product ownership before launch, but not treated as a vulnerability here — the real problem is finding #1 above (writable/readable *credentials*), not readable public content.

---

## Suggested fix order before launch

1. Lock down `globals/Settings` credential fields (finding #1) and fix the tenant-access bypass + add real roles (finding #2) — these two together mean the app currently has no meaningful access control boundary.
2. Add a CI gate (lint/typecheck/test) to the deploy workflow and wire `payload migrate` into the release path (findings #3, #4).
3. Add `DATABASE_URI` validation, a health-check endpoint, and `error.tsx`/`not-found.tsx` (findings #5, #6, #10) — cheap, high-value production-hardening.
4. Everything else in High/Medium can reasonably follow in the first post-launch iteration.