# Phase 0 — fresh code review: triaged findings (2026-07-09)

Multi-agent review of the **agent, webhook, and send paths** at agent-unify
HEAD (`99b68a6`'s parent `cb7e225`), per target picture §9.11. Five dimension
finders + one adversarial verifier per finding (22 agents total).
**15 findings confirmed, 2 refuted.** Verifier reasoning is preserved in the
workflow journal; every confirmed finding below survived an explicit
refutation attempt against the real code.

Triage buckets:
- **fix-now** — small, contained fixes to land as a hotfix batch at the very
  start of Phase 1, before any rebuild work (no product code in Phase 0).
- **fix-in-phase** — absorbed by the rethink phase that rebuilds that area;
  named per finding.
- **refuted** — verified not-a-defect; recorded so it isn't re-reported.

## Fix-now (hotfix batch, Phase 1 start)

| # | finding | file | sev | fix sketch |
|---|---|---|---|---|
| 1 | **Login rate limiter bypassable via X-Forwarded-For spoofing** — guard keys on the *first* XFF entry, which nginx leaves attacker-controlled; rotating a fake IP per request = unlimited password guessing. Test pins the wrong behaviour. | `apps/api/src/auth/login-rate-limit.guard.ts:18` | high | Trust `req.ip` (Fastify `trustProxy`) or take the *last* XFF hop; update the test. |
| 2 | **Webhook events stored but never enqueued are permanently lost** — row is created before `queue.add`; on Redis failure Meta's retry hits the dedupKey P2002 and is acked as duplicate. Invalid-signature rows also claim dedup keys (forged unsigned envelope can pre-claim a predictable IG `comment:<id>`). No reconciliation sweep exists. | `apps/api/src/webhooks/webhook-ingest.service.ts:83` | high | On P2002 load the row; if valid + unprocessed + no job, re-enqueue before acking. Don't let `signatureValid=false` rows claim dedup keys. |
| 3 | **Re-scheduling a campaign leaves the old delayed start job → fires at the original earlier time** — `schedule()` allows re-schedule but never removes the prior delayed job; `start()`'s status guard passes. Broadcast sends hours early, silently. | `apps/api/src/campaigns/campaign-send.service.ts:260` | high | Stable `jobId: campaign-start:<id>` + remove-before-add (the resume queue already uses this pattern), and/or `start()` verifies `scheduledAt <= now`. |
| 4 | **Reminder sends have zero idempotency across BullMQ retries** — any failure after the API-side Meta send (worker 10s abort vs Meta 15s, or a post-send DB error) re-sends the same reminder up to 5×. | `apps/api/src/campaigns/batch-send.service.ts:78` | high | Dedupe id per `ReminderSendItem` recorded before return; raise the internal timeout (with #5). |
| 5 | **Worker internal-API timeout (10s) < Meta send timeout (15s)** — abandoned-but-still-running sends get retried; campaign recipients can double-send when Meta latency ≥ ~14.5s or on a post-send DB error / restart. | `apps/worker/src/lib/internal-api.ts:14` | med | Raise `INTERNAL_API_TIMEOUT_MS` above the Meta ceiling (≥30s); atomic status-claim `UPDATE … WHERE status='pending'` in `sendOneRecipient`. |
| 6 | **Variable-mapping path `waId` resolves against a renamed field** — resolver reads `contact["waId"]` but Prisma now exposes `channelUserId`; a campaign mapped to "WhatsApp ID" skips 100% of recipients (UI actively offers it). Unit tests mock the stale shape and mask it. | `packages/shared/src/campaigns/resolve-mapping.ts:25` | med | Alias `waId` → `channelUserId` at the resolver input (both call sites) or in `readPath`; fix the test fixtures. |
| 7 | **Envelope dedup keys only the first item of a batched POST** — a retried delivery batched with newer events drops the newer ones as "duplicate", acked but never stored. Composes badly with #2. | `packages/shared/src/meta/webhook-parser.ts:57` | med | Derive the key over *all* event ids in the envelope (per-wamid idempotency downstream already makes overlap safe). |
| 8 | **JSON body `null` crashes ingest with an unauthenticated 500** — `JSON.parse("null")` passes the try/catch, then `env.entry?.[0]` throws on null before the store-everything write. | `apps/api/src/webhooks/webhook-ingest.service.ts:64` | low | Guard: non-null object or fall back to `{}` (mirror the catch branch). |

## Fix-in-phase

| # | finding | file | sev | phase / where it's absorbed |
|---|---|---|---|---|
| 9 | **Per-contact turn queue releases the next turn before the reply is sent/persisted** — a burst's second turn reads history missing the first reply → duplicate/overlapping answers, out-of-order sends. The exact bug class the queue's comment claims to remove. | `apps/api/src/agent/agent.service.ts:116` | med | **Phase 1** (agent rebuild — turn-trace persistence reworks this exact path). Deliver-inside-the-queue via callback or chained send. |
| 10 | **`crm_lead` profile-name fallback clobbers a collected real name** — fallback substitutes the WhatsApp display alias *before* the merge, so later calls omitting `name` overwrite "Ahmed" with "R7 🏠 Dubai", which then propagates into the LEAD PROFILE prompt as "never re-ask". | `packages/shared/src/agent/tools.ts:257` | med | **Phase 1**. Persist raw input to `saveLeadState`; apply the fallback only to the CRM payload. |
| 11 | **Agent reply send result ignored — structured send failure = customer silence** — `MessageService.send` never throws, so the "never silence" catch can't fire; a failed interactive send (e.g. clamp() splitting a surrogate-pair emoji) or token-expiry window silently drops replies while returning `handled: true`. | `apps/api/src/flows/basic-flow-engine.ts:246` | low | **Phase 1**. Check `MetaSendResult`; failed interactive → retry as plain text; failed text → error log. Fix `clamp()` to be codepoint-safe while there. |
| 12 | **`handleInbound`'s wamid early-return turns BullMQ retries into no-ops** — a transient failure after `message.create` (opt-out hook, media enqueue, flow enqueue) is never recovered: retries find the row and skip; the retry then marks the event processed, masking the drop. | `packages/shared/src/meta/envelope-processor.ts:254` | med | **Phase 1 or 3** (webhook surface work). Guard should *resume* (idempotent re-enqueue via `jobId = message id`), and wrap `onOptOut` like `onInboundMessage`. |
| 13 | **Campaign stuck in `sending` forever when a send job exhausts its 5 attempts** — 130429 outlasting ~75s leaves rows `pending` with no sweep, no failure mark; completion never fires. (Manual pause→resume does recover — undiscoverable.) | `apps/api/src/campaigns/campaign-send.service.ts:879` | med | **Phase 2** (Home/alerting — "campaign stuck" is a designed Home signal; add the final-attempt failure mark + stale-pending sweep then). |
| 14 | **`apiSend` dedupeKey is check-then-act** — two concurrent deliveries with the same key both pass and both send (the exact retry-storm case dedupeKey documents itself as solving). | `apps/api/src/campaigns/campaign-send.service.ts:620` | med | **Phase 2/3**, but **before the next real event campaign**: reserve the row (`clientRef` + status `sending`) before the Meta send. |
| 15 | **Two gate admin surfaces clobber each other's whole-blob `channelGates` writes** — last-writer-wins upsert re-persists the other channel's gate from a stale read; can silently revert WhatsApp to live. Narrow window, admin-only. | `apps/api/src/agent/agent-admin.controller.ts:177` | med | **Phase 1** (Agent rebuild reworks gate UI/API). Per-surface patch in a transaction / optimistic concurrency. |

## Refuted (recorded so they don't come back)

- **Lazy migration never neutralizes the legacy `agent` key (fail-open to stale live gate)** — mechanics accurate, but no code path at HEAD can write a malformed mode, the raw-settings misuse required is privileged-operator-equivalent, and the fallback is pinned as *intended* by test + docstring. Worth revisiting as hardening when gates are reworked in Phase 1 (converge legacy rows on first write), not a defect.
- **Webhook verify-token compared non-constant-time** — timing channel unmeasurable (per-request DB read + AES decrypt drowns it), and the token yields nothing anyway (POST ingestion is independently HMAC-gated). Hardening nit only.

## Notes for the Phase 1 plan

- The fix-now batch is deliberately small and mechanical; each fix gets a
  failing test first (several findings note existing tests *pin the wrong
  behaviour* — those tests get corrected, not deleted).
- Findings 9/10/11 all live in the exact surface Phase 1's turn-trace work
  rebuilds — fix them as part of that work, with trace-visible evidence.
- Finding 2+7 (webhook ingest family) should land together; they compose.
- Full verifier transcripts: workflow run `wf_4fa493ca-d7e` (session journal).