Ad Network Data Layer: Multi-Angle Review + Full Fix Pass

Date: 2026-07-23 Branch: claude/ad-network-rotation-data-gs1sjaRelated issues: #692, #694, #695, #696, #697 Prior work: phases A-D of the ad network data layer plan (docs/planning/plans/2026-07-22-ad-network-data-layer.md, commits 4b8e0f7, b3436cd, c7eb459, 0ba8656, 4a17e86).

What happened

Before flipping anything on, an 8-angle background review (correctness, security/privacy, BigQuery SQL, React/UI state, API contracts, test quality, conventions, cross-cutting data flow) swept the entire ad delivery pipeline. It reported 22 verified findings; all were fixed in this pass and the review was then re-run over the fix diff.

The headline findings and their fixes

1. Merchant identity mismatch (critical). Every merchant-scoped analytics read bound req.user.uid as merchant_id, but merchant identity is the minted merchants/{id} doc id (see generateMerchantId in the auth API), linked via users/{uid}.merchantId. The uid never matches a rollup row, so merchants would have seen permanently empty dashboards, and the web app's offers fetch 403'd against merchants-api. Fix: requireRole attaches the linked merchantId, resolveMerchantScope pins merchants to it (403 with a clear message when unlinked), admins may pass ?merchantId, and the web app resolves getOwnMerchantId() from the user doc. Recorded as decision D13.
2. Spoofable billing events. The tracking route stamps client submissions metadata.attributionSource='client_untrusted'; claims/redemptions are billing-adjacent and must originate server-side, so the rollup MERGE now excludes client-untrusted rows for those two event names only (fills, impressions, and clicks are client-observed by nature). Decision D15.
3. Truncation-blind totals / suppressed-cell leak surface. Totals were summed client-side over a LIMIT-capped, gated row list. Now a second SQL aggregate computes totals over ALL matching cells with the k-anon floor applied per cell (SUM(IF(unique_users >= @floor, col, 0))), plus a suppressed-cell count; the row list carries truncated when capped. Decision D14.
4. Cross-user dedupe bleed. The fill/impression dedupe window survived sign-out, so a second user on a shared device inherited "already counted" keys. noteAdTelemetryUser(uid) (called from App.jsx's auth listener) resets the window on uid change.
5. Self-healing aggregation. Scheduled MERGE default lookback moved from 1 to 3 days; the MERGE target is day-bounded for partition pruning; the admin backfill body is strictly validated (lookbackDays integer 1-365, unknown keys rejected). Decision D16.

Plus 17 smaller fixes: click emitters migrated to trackAdClick (full attribution payload), stale merchant-portal state resets on merchant switch (Overview band, OffersList delivery map), RFC 4180 CSV quoting, loader try/finally so the page can't strand on "loading", one shared formatter/floor-constant module (@lantern/shared/ads/delivery plus apps/admin/.../dashboardFormat.js) replacing three local copies, zod schema and PLACEMENTS dedupe across routes, METRIC_EVENTS built from the canonical AD_EVENTS constants with a registry-membership test, a 60s client cache on delivery fetches, emoji error banners replaced with AlertTriangle, row keys including merchant_id, and em dash glyphs replaced with en dashes (five files dropped from the ratchet baseline, none added).

Verification

• Analytics API: service + route specs updated for the two-query fetch, gated totals, minted-id scoping, admin ?merchantId, and the 403 unlinked path.
• Web: CSV quoting spec (including the quote-doubling case), telemetry user-switch spec, delivery service specs against the async auth shape.
• npm run validate full suite green before push; openapi.json and the plan doc runbook updated to match the shipped behavior.

Round 2: the re-review (8 fresh angles over the fix diff)

The re-run came back with a second wave of verified findings, all fixed in the follow-up commit (decisions D17-D20 in the plan doc):

1. Environment tag mismatch (critical, dev-blocking). Dev Cloud Run deploys set NODE_ENV=production, so resolveEnvironment() filtered environment='production' against rows the forge stamps 'development': every BQ dashboard would read zero rows on dev, silently. Reads now derive the tag from the project id exactly like the write side. (D17)
2. Unproxied web path. The new /api/merchants/{id}/offers fetch matched no proxy route in _worker.js or the Vite dev proxy, fell through to the SPA fallback, and soft-failed to "no offers" forever. Both proxies now route /api/merchants.
3. NULL-stamp trust filter. NOT (...='client_untrusted') also dropped NULL-stamped claim/redeem rows via SQL three-valued logic. Rewritten as a NULL-safe fail-closed allowlist (attributionSource='server_authoritative'), and the attribution contract now requires that stamp + user_id from the future server-side claim flow. (D18)
4. Dedupe-window wipes. The first auth callback of a page load (and a guest converting to an account) cleared the fill/impression dedupe keys, re-introducing the #692 inflation for pre-auth fills; the window now clears only when a DIFFERENT signed-in user appears.
5. Cross-merchant cache leak. The 60s delivery cache was keyed by path alone; merchant B on a shared browser inside the TTL could be served merchant A's numbers. Cache keys now carry the merchant scope, and the Ad Delivery dashboard's Refresh forces a real fetch (it previously served the cache while stamping "Updated: now"); range switches drop stale data so old numbers can't sit under new labels, and truncated coverage counts label themselves as a floor. (D20)
6. Click-only slots. Feed (AdSlot shim) and venue-detail emitted clicks but never FILLED/IMPRESSION, creating impossible cells that inflate CTR. Both slots now emit the full fill / in-view impression / click triplet, and the venue page credits placement 'inline' rather than polluting hero cells. (D19)
7. Test gaps closed: rbac merchantId attach, the merchant service fetches (minted-id URL + SPA-fallback soft-fail), truncation flag true-path, outage dashes (never fabricated zeros), and a full route spec for the strict backfill body. One accepted gap: no App-level integration test renders the real auth listener to assert the telemetry user-note wiring (unit-tested at module level; App harness cost not justified).

Follow-ups

• Flip-on is now automatic: the dev deploy workflow creates the rollup table and arms the analytics-ad-delivery-daily scheduler job on the first analytics-api deploy after merge (idempotent; plan doc section 4b). The remaining runbook steps are verification only.
• Claim/redeem server-side flow (#697) is the natural next theme; the trust filter and attribution contract are already in place for it.