From 6bb7314a5e63e5193d4fbc5bf962c7b21338be9b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 14:22:50 +0000 Subject: [PATCH 01/11] Compute the guest guidebook's clock in the property's timezone, not Eastern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both guest-facing guidebook pages worked out "what time is it for this guest" in a hardcoded America/New_York, for every property in the country. properties.timezone was not even in the select. Two guest-visible consequences, both worst in the evening: • computeStay(). Once Eastern rolls past midnight, `today >= checkoutDate` fires and the guidebook shows CHECKOUT instructions to a guest who is still mid-stay. One hour early in Central, two in Mountain, three in Pacific, five in Hawaii. • hourOfDay(). It feeds getActiveSlotTypes(), which decides which SPONSOR SLOTS render. Sponsors pay for that placement, so the wrong hour shows the wrong paying businesses — and the entire 5pm–8pm evening dining window on the west coast reads as 8pm–11pm. It also drives the sky state and the breakfast/evening recommendation bands, and on the slug page (which has no booking) it is the sole input to the arrival-vs-mid phase. Live, not theoretical: production is already 4 of 27 properties in America/Chicago, and the error only grows as the product moves west. FALLBACK_TIMEZONE stays, but only as a fallback for a property somehow lacking one, with a comment saying it is not the default and why that distinction cost something. The test pins the boundary directly — 2026-08-11T04:30:00Z is 11:30pm Aug 10 in Chicago and 12:30am Aug 11 in New York — and deliberately keeps an assertion that the SAME instant still flips under America/New_York. Without it, a fixture that failed to straddle a date boundary would pass for the wrong reason and prove nothing. Hawaii covers the widest US offset, so the case does not read as a one-hour rounding curiosity. Verified: next build exits 0 (BUILD_ID confirmed from that run), tsc clean, 3048 unit + 112 component tests pass, lint 192 at the ceiling, check:ui-classes clean, semgrep chokepoints exit 0, ratchet vs. the real PR base exit 0, no ratchet count increased. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- app/g/[slug]/page.tsx | 8 ++- app/g/b/[token]/page.tsx | 24 +++++-- unit/guidebook/guest-stay-timezone.test.ts | 82 ++++++++++++++++++++++ 3 files changed, 106 insertions(+), 8 deletions(-) create mode 100644 unit/guidebook/guest-stay-timezone.test.ts diff --git a/app/g/[slug]/page.tsx b/app/g/[slug]/page.tsx index e40476d1..88c7eacc 100644 --- a/app/g/[slug]/page.tsx +++ b/app/g/[slug]/page.tsx @@ -9,6 +9,7 @@ import type { GuidebookSponsorView } from '@/components/guidebook/guest-guideboo import type { GuidebookSponsor, GuidebookPropertyConfig, Property } from '@/types/database' import { unwrap, unwrapList } from '@/lib/supabase/unwrap' +/** Only for a property with no timezone — see the note in app/g/b/[token]/page.tsx. */ const FALLBACK_TIMEZONE = 'America/New_York' // A guidebook's active sponsor slots are a small, curated set per org; the @@ -30,7 +31,7 @@ function heroPhotoUrl(path: string | null | undefined): string | null { const CONFIG_FIELDS = ` id, slug, wifi_network, wifi_password, check_in_instructions, check_out_instructions, house_rules, is_published, org_id, - properties(id, name, address, lat, lng, checkin_time, checkout_time) + properties(id, name, address, lat, lng, timezone, checkin_time, checkout_time) ` const getGuidebookConfig = cache(async (slug: string) => { @@ -114,8 +115,9 @@ export default async function GuestGuidebookPage({ const sponsors = unwrapList(sponsorsRes, { site: 'page.g.slug', orgId: config.org_id }) const hourOfDay = Number( - new Intl.DateTimeFormat('en-US', { hour: 'numeric', hour12: false, timeZone: FALLBACK_TIMEZONE }) - .format(new Date()) + new Intl.DateTimeFormat('en-US', { + hour: 'numeric', hour12: false, timeZone: property.timezone || FALLBACK_TIMEZONE, + }).format(new Date()) ) const weather = property.lat && property.lng diff --git a/app/g/b/[token]/page.tsx b/app/g/b/[token]/page.tsx index 40abf856..09bb2e3c 100644 --- a/app/g/b/[token]/page.tsx +++ b/app/g/b/[token]/page.tsx @@ -8,6 +8,15 @@ import { GuidebookUnavailable } from '@/components/guidebook/guidebook-unavailab import type { GuidebookSponsorView } from '@/components/guidebook/guest-guidebook-view' import type { GuidebookSponsor, GuidebookPropertyConfig, Property } from '@/types/database' +/** + * Used only when a property somehow has no timezone. It is NOT the default: + * computing "what time is it for this guest" in Eastern for every property in + * the country is how a Central property's guidebook reads midnight at 11pm + * local — and hourOfDay selects which SPONSOR SLOTS are shown, so the wrong + * hour shows the wrong paying sponsors. Production is already 4 of 27 + * properties in America/Chicago, and the error grows westward: two hours in + * Mountain, three in Pacific, five in Hawaii. + */ const FALLBACK_TIMEZONE = 'America/New_York' const SUPABASE_URL = process.env.NEXT_PUBLIC_SUPABASE_URL @@ -24,10 +33,13 @@ function heroPhotoUrl(path: string | null | undefined): string | null { type StayPhase = 'arrival' | 'mid' | 'checkout' -function computeStay(checkinDate: string, checkoutDate: string): { +export function computeStay(checkinDate: string, checkoutDate: string, timeZone: string): { phase: StayPhase; nightIndex: number; totalNights: number } { - const today = new Intl.DateTimeFormat('en-CA', { timeZone: FALLBACK_TIMEZONE }).format(new Date()) // YYYY-MM-DD + // The guest's local date, not the server's and not Eastern's. On checkout + // eve a Central property crossed into `checkout` phase an hour early, so the + // guidebook showed checkout instructions to a guest still mid-stay. + const today = new Intl.DateTimeFormat('en-CA', { timeZone }).format(new Date()) // YYYY-MM-DD const totalNights = Math.max(1, Math.round( (Date.parse(checkoutDate) - Date.parse(checkinDate)) / 86_400_000 )) @@ -42,7 +54,7 @@ function computeStay(checkinDate: string, checkoutDate: string): { const CONFIG_FIELDS = ` id, slug, wifi_network, wifi_password, check_in_instructions, check_out_instructions, house_rules, is_published, org_id, - properties(id, name, address, lat, lng, checkin_time, checkout_time) + properties(id, name, address, lat, lng, timezone, checkin_time, checkout_time) ` const getGuidebookData = cache(async (token: string) => { @@ -139,8 +151,10 @@ export default async function GuestBookingGuidebookPage({ .eq('org_id', booking.org_id) .eq('status', 'active') + const timeZone = property.timezone || FALLBACK_TIMEZONE + const hourOfDay = Number( - new Intl.DateTimeFormat('en-US', { hour: 'numeric', hour12: false, timeZone: FALLBACK_TIMEZONE }) + new Intl.DateTimeFormat('en-US', { hour: 'numeric', hour12: false, timeZone }) .format(new Date()) ) @@ -174,7 +188,7 @@ export default async function GuestBookingGuidebookPage({ hourOfDay={hourOfDay} weather={weather} heroPhotoUrl={heroPhotoUrl(config.hero_photo_storage_path)} - stay={computeStay(booking.checkin_date, booking.checkout_date)} + stay={computeStay(booking.checkin_date, booking.checkout_date, timeZone)} bookingToken={token} extensionRequest={extensionRequest ?? null} extensionConfig={ diff --git a/unit/guidebook/guest-stay-timezone.test.ts b/unit/guidebook/guest-stay-timezone.test.ts new file mode 100644 index 00000000..68b51673 --- /dev/null +++ b/unit/guidebook/guest-stay-timezone.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, vi, afterEach } from 'vitest' +import { computeStay } from '@/app/g/b/[token]/page' + +// ============================================================================ +// The guest guidebook computed "what day is it for this guest" — and, next to +// it, "what hour is it" — in America/New_York for EVERY property, ignoring +// properties.timezone entirely (the column was not even selected). +// +// Two guest-visible consequences, both worst in the evening: +// +// • computeStay: once Eastern rolls past midnight, `today >= checkoutDate` +// fires, so a guest still mid-stay is shown CHECKOUT instructions. One +// hour early in Central, two in Mountain, three in Pacific, five in Hawaii. +// • hourOfDay: feeds getActiveSlotTypes(), which decides WHICH SPONSOR SLOTS +// render. Sponsors pay for that placement, so the wrong hour shows the +// wrong paying businesses — and the whole 5pm–8pm evening dining window on +// the west coast reads as 8pm–11pm. +// +// Production already has 4 of 27 properties in America/Chicago, so this is +// live, not theoretical — and the error only grows as the product moves west. +// ============================================================================ + +afterEach(() => vi.useRealTimers()) + +/** Freezes wall-clock at a real instant so timezone maths is deterministic. */ +function at(iso: string) { + vi.useFakeTimers() + vi.setSystemTime(new Date(iso)) +} + +describe('computeStay uses the property timezone, not Eastern', () => { + // 2026-08-11T04:30:00Z is 11:30pm Aug 10 in Chicago and 12:30am Aug 11 in + // New York. A guest checking out on Aug 11 is still mid-stay in Chicago. + it('does not flip a Central guest to checkout while it is still checkout eve', () => { + at('2026-08-11T04:30:00Z') + + const central = computeStay('2026-08-08', '2026-08-11', 'America/Chicago') + expect(central.phase, 'still the last night of the stay in Chicago').toBe('mid') + }) + + // The same instant in the timezone the code used to hardcode. Keeping this + // alongside proves the fixture actually straddles a date boundary rather + // than passing for some unrelated reason. + it('would have flipped under the old hardcoded Eastern behaviour', () => { + at('2026-08-11T04:30:00Z') + + const eastern = computeStay('2026-08-08', '2026-08-11', 'America/New_York') + expect(eastern.phase, 'Eastern has already rolled to checkout day').toBe('checkout') + }) + + it('reaches checkout once the guest\'s own day arrives', () => { + at('2026-08-11T14:00:00Z') // 9am Chicago on checkout day + + expect(computeStay('2026-08-08', '2026-08-11', 'America/Chicago').phase).toBe('checkout') + }) + + it('reports arrival before check-in day in the property timezone', () => { + at('2026-08-08T02:00:00Z') // 9pm Aug 7 in Chicago + + const stay = computeStay('2026-08-08', '2026-08-11', 'America/Chicago') + expect(stay.phase).toBe('arrival') + expect(stay.nightIndex).toBe(0) + expect(stay.totalNights).toBe(3) + }) + + // Hawaii is the widest US offset and the case that shows the bug is not a + // one-hour rounding curiosity. + it('is five hours out from Eastern at the extreme', () => { + at('2026-08-11T06:00:00Z') // 8pm Aug 10 in Honolulu, 2am Aug 11 in New York + + expect(computeStay('2026-08-08', '2026-08-11', 'Pacific/Honolulu').phase).toBe('mid') + expect(computeStay('2026-08-08', '2026-08-11', 'America/New_York').phase).toBe('checkout') + }) + + it('counts nights from the dates, independent of timezone', () => { + at('2026-08-09T18:00:00Z') + + for (const tz of ['America/New_York', 'America/Chicago', 'Pacific/Honolulu']) { + expect(computeStay('2026-08-08', '2026-08-11', tz).totalNights, tz).toBe(3) + } + }) +}) From eff4a3b0c707d55e749c245f01c2393d426f1e17 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 14:51:17 +0000 Subject: [PATCH 02/11] Anchor the guest SMS correction window on a timestamp the upsert cannot move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings in optInGuestSms. The consent LOGIC around them is careful — STOP honoured globally by phone across every org and booking, re-consent restricted to the handset, revocation records kept forever, degraded consent reads failing closed — which is what made these two stand out. 1. The 15-minute number-correction window was measured from `opted_in_at`, which the upsert REFRESHES on every submission. So it was never "15 minutes from the opt-in" as documented; it was 15 minutes from the LAST submission, and resubmitting just inside it restarted the clock. The window could be walked forward without limit — precisely the days-later repoint the comment above it says a leaked guidebook link enables. It now anchors on `created_at`, which the upsert never names and which therefore keeps the original row's timestamp through every conflict-update. No migration needed; the immutable anchor was already there. Both existing window tests had `opted_in_at` fixtures. Worth noting that the refusal test did not simply need updating — with `created_at` absent it fell back to epoch 0 and PASSED for entirely the wrong reason, which is the shape a fixture change can hide behind. Both now set `created_at`, and a third case covers the walk-forward directly: original opt-in 24h ago, resubmitted a minute ago, repoint refused. 2. The booking lookup discarded its error. The two consent reads that follow it both fail closed with an explicit note about why; this one did not, so a transient failure was indistinguishable from a bad token and a guest holding a perfectly valid link was told the link was invalid — with nothing logged and nothing reported. supabase-error-handling baseline for this file 3 -> 2. Also logs FUTURE_REMEDIATION item 24: the opt-in stores no EVIDENCE of the consent it relies on — no disclosure text or version, no IP or user agent, no record of which token was used. Under TCPA the burden of proving prior express written consent sits with the sender, and the strongest artefact available today is a row saying a number opted in at a timestamp. Not fixed here because it needs a migration and, more to the point, storing request metadata against a phone number is a PII expansion that needs its own retention rule — a decision for whoever owns the compliance posture, not a default to pick mid-audit. Two canaries, each reverted with the edit verified before the suite ran. Verified: next build exits 0 (BUILD_ID confirmed from that run), tsc clean, 3050 unit + 112 component tests pass, lint 192 at the ceiling, check:ui-classes clean, semgrep chokepoints exit 0, ratchet vs. the real PR base exit 0, no ratchet count increased. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- FUTURE_REMEDIATION.md | 52 +++++++++++++++++ app/actions/guidebook.ts | 32 ++++++++-- .../supabase-error-handling.test.ts | 5 +- unit/guidebook/guidebook-actions.test.ts | 58 ++++++++++++++++++- 4 files changed, 140 insertions(+), 7 deletions(-) diff --git a/FUTURE_REMEDIATION.md b/FUTURE_REMEDIATION.md index 4fd98505..4ab0502b 100644 --- a/FUTURE_REMEDIATION.md +++ b/FUTURE_REMEDIATION.md @@ -1106,3 +1106,55 @@ turnover is not in the assigned set at all — at which point the existing safe today (assets are pulled narrow and pruned wide, so they are over-retained, never lost), but once the assigned set is bounded, all three should be derived from one helper rather than three hand-rolled sets. + +--- + +## 24. Guest SMS opt-in stores no evidence of the consent it relies on + +**File:** `app/actions/guidebook.ts` (`optInGuestSms`), table +`guidebook_guest_sms_optins` + +Found during the guidebook audit (2026-08-07). Not fixed here: it needs a +migration, and what to retain is a legal question rather than an engineering +one. + +The consent LOGIC is careful — STOP is honoured globally by phone across every +org and booking, re-consent is restricted to the handset (START/YES/UNSTOP), +the revocation record is deliberately kept forever by +`guest-pii-retention`, and a degraded consent read fails closed. What is +missing is the EVIDENCE. + +`guidebook_guest_sms_optins` holds `phone_e164`, `opted_in_at`, +`opted_out_at`, `is_active` and the send bookkeeping — and nothing about the +consent event itself: + +- no record of the disclosure text the guest was actually shown (it lives only + in `app/g/b/[token]/opt-in/opt-in-client.tsx`, and changing that copy leaves + no trace of what earlier opt-ins agreed to) +- no IP address or user agent +- no record of which guidebook token was used + +Under TCPA the burden of proving prior express written consent sits with the +sender. Today the strongest evidence FieldStay could produce for a disputed +opt-in is a row saying a number opted in at a timestamp — with no way to show +what was on screen when it happened, and no way to distinguish the guest from +anyone else holding the booking's guidebook link. + +**Why it was not just added.** Storing an IP and user agent against a phone +number is itself a PII expansion, and `guest-pii-retention` currently deletes +opt-in rows (except revocations) on a schedule — so consent evidence would +need its own retention rule, probably a longer one than the PII it sits +beside. That is a decision for whoever owns the compliance posture, not a +default to pick while auditing. + +**If it is taken on**, the shape that fits the existing design: a +`consent_disclosure_version` (or the literal text hash) written alongside +`opted_in_at`, plus request metadata, and an explicit carve-out in +`lib/inngest/functions/cron/guest-pii-retention.ts` so consent records outlive +the guest data they attest to — mirroring how revocations are already exempt +from that sweep. + +**Adjacent, already fixed 2026-08-07:** the 15-minute number-correction window +was measured from `opted_in_at`, which the upsert refreshes on every +submission, so the window restarted on each resubmit and could be walked +forward indefinitely. It now anchors on the immutable `created_at`. diff --git a/app/actions/guidebook.ts b/app/actions/guidebook.ts index a0bf0072..41c97a2c 100644 --- a/app/actions/guidebook.ts +++ b/app/actions/guidebook.ts @@ -355,6 +355,12 @@ export async function updateStayExtensionSettings( * How long after the original opt-in a guest may correct the number they * entered. Long enough to notice a typo and resubmit; far short of the span * over which a guidebook link circulates. + * + * Measured from guidebook_guest_sms_optins.created_at, which the upsert never + * writes, rather than opted_in_at, which it refreshes on every submission. + * From opted_in_at the window was not 15 minutes from the opt-in at all — it + * was 15 minutes from the LAST submission, so resubmitting inside it restarted + * the clock and the window could be walked forward without limit. */ const OPTIN_CORRECTION_WINDOW_MS = 15 * 60 * 1000 @@ -373,12 +379,24 @@ export async function optInGuestSms( const supabase = createServiceClient({ publicSurface: 'guidebook-guest-sms-optin' }) - const { data: booking } = await supabase + // Binds its error, like the two consent reads below it. Discarding it made + // a transient failure indistinguishable from a bad token, so a guest + // holding a perfectly valid link was told the link was invalid — with + // nothing logged and nothing reported. The two reads that follow this one + // both already failed closed with an explicit note about why; this was the + // odd one out. + const bookingRes = await supabase .from('bookings') .select('id, org_id, property_id') .eq('guidebook_token', guidebookToken) .maybeSingle() + if (bookingRes.error) { + console.error('[optInGuestSms] booking lookup', bookingRes.error.message) + reportError(bookingRes.error, { site: 'serverAction.guidebook.optInGuestSms.booking' }) + return { error: 'Something went wrong. Please try again.' } + } + const booking = bookingRes.data if (!booking) return { error: 'Invalid guidebook link.' } // ── Consent gate 1: has this NUMBER revoked consent anywhere? ──────────── @@ -424,9 +442,15 @@ export async function optInGuestSms( // corrections stay open for a short window after the original opt-in. That // covers the real case — a typo is noticed immediately — while refusing a // repoint days later, which is what a leaked link enables. + // created_at, not opted_in_at. The upsert below REFRESHES opted_in_at on + // every submission, so a window measured from it walks forward: repoint at + // 14 minutes, and the 15-minute clock restarts from there, indefinitely. + // created_at carries the original row's timestamp and the upsert never + // names it, so it is the only immutable anchor here — and "how long after + // the ORIGINAL opt-in" is what this window is documented to mean. const existingRes = await supabase .from('guidebook_guest_sms_optins') - .select('id, phone_e164, opted_in_at') + .select('id, phone_e164, created_at') .eq('booking_id', booking.id) .maybeSingle() @@ -437,8 +461,8 @@ export async function optInGuestSms( const existing = existingRes.data if (existing && existing.phone_e164 !== phoneE164) { - const optedInAt = existing.opted_in_at ? new Date(existing.opted_in_at).getTime() : 0 - if (Date.now() - optedInAt > OPTIN_CORRECTION_WINDOW_MS) { + const firstOptedInAt = existing.created_at ? new Date(existing.created_at).getTime() : 0 + if (Date.now() - firstOptedInAt > OPTIN_CORRECTION_WINDOW_MS) { // Never echo either number back — guest PII, and confirming which // number is on file would itself be a disclosure. return { error: 'A different number is already signed up for this stay. Contact your host to change it.' } diff --git a/unit/guardrails/supabase-error-handling.test.ts b/unit/guardrails/supabase-error-handling.test.ts index 57d4052d..3d4a7cbc 100644 --- a/unit/guardrails/supabase-error-handling.test.ts +++ b/unit/guardrails/supabase-error-handling.test.ts @@ -106,7 +106,10 @@ const BASELINE: Record = { 'app/(dashboard)/templates/inventory/actions.ts': 2, 'app/(dashboard)/templates/maintenance/actions.ts': 3, 'app/(dashboard)/vendors/actions.ts': 3, - 'app/actions/guidebook.ts': 3, + // 3 -> 2: optInGuestSms's booking lookup now binds and reports its error. + // Discarding it made a transient failure indistinguishable from a bad token, + // so a guest with a valid link was told the link was invalid. + 'app/actions/guidebook.ts': 2, 'app/api/repuguard/generate/route.ts': 3, 'app/api/vendor-connect/[token]/onboard/route.ts': 2, diff --git a/unit/guidebook/guidebook-actions.test.ts b/unit/guidebook/guidebook-actions.test.ts index 30c44c01..cd9dbff3 100644 --- a/unit/guidebook/guidebook-actions.test.ts +++ b/unit/guidebook/guidebook-actions.test.ts @@ -424,7 +424,7 @@ describe('actions/guidebook', () => { data: { id: 'optin_1', phone_e164: '+12065551234', - opted_in_at: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), + created_at: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), }, error: null, }, @@ -453,7 +453,7 @@ describe('actions/guidebook', () => { data: { id: 'optin_1', phone_e164: '+12065551234', - opted_in_at: new Date(Date.now() - 60 * 1000).toISOString(), // 1 min ago + created_at: new Date(Date.now() - 60 * 1000).toISOString(), // 1 min ago }, error: null, }, @@ -467,6 +467,60 @@ describe('actions/guidebook', () => { expect(result).toEqual({ success: true }) }) + // The window is anchored to created_at, which the upsert never writes — + // not opted_in_at, which it REFRESHES on every submission. Measured from + // opted_in_at it was never "15 minutes from the opt-in" at all: it was 15 + // minutes from the LAST submission, so resubmitting just inside it + // restarted the clock and the window could be walked forward without + // limit — exactly the days-later repoint the comment says a leaked link + // enables. + it('refuses a repoint 24h after the original opt-in even if the row was resubmitted a minute ago', async () => { + vi.mocked(normalizePhoneToE164).mockReturnValue('+12065559999') + const supabase = makeSupabase({ + bookings: [{ data: { id: 'booking_1', org_id: 'org_1', property_id: 'prop_1' } }], + guidebook_guest_sms_optins: [ + { data: null, error: null }, + { + data: { + id: 'optin_1', + phone_e164: '+12065551234', + // Original opt-in a day ago … + created_at: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), + // … but refreshed a minute ago, which used to reopen the window. + opted_in_at: new Date(Date.now() - 60 * 1000).toISOString(), + }, + error: null, + }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + const result = await optInGuestSms('valid-guidebook-token', '(206) 555-9999') + + expect(result).toEqual({ + error: 'A different number is already signed up for this stay. Contact your host to change it.', + }) + expect(inngest.send).not.toHaveBeenCalled() + }) + + // Two of the three reads in this action already failed closed with an + // explicit note about why. The booking lookup discarded its error, so a + // transient failure was indistinguishable from a bad token and a guest + // holding a valid link was told the link was invalid. + it('fails closed, not "invalid link", when the booking lookup itself errors', async () => { + vi.mocked(normalizePhoneToE164).mockReturnValue('+12065551234') + const supabase = makeSupabase({ + bookings: [{ data: null, error: { message: 'deadlock detected', code: '40P01' } }], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + const result = await optInGuestSms('valid-guidebook-token', '(206) 555-1234') + + expect(result).toEqual({ error: 'Something went wrong. Please try again.' }) + expect(JSON.stringify(result)).not.toMatch(/invalid guidebook link/i) + expect(inngest.send).not.toHaveBeenCalled() + }) + it('rejects an invalid/unrecognized guidebook token before writing anything (IDOR/token check)', async () => { vi.mocked(normalizePhoneToE164).mockReturnValue('+12065551234') const supabase = makeSupabase({ bookings: [{ data: null }] }) From 9191742660982918c2b06247b163010de9a7b3da Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 15:17:16 +0000 Subject: [PATCH 03/11] fix(guidebook): release SMS claims when the send throws, not only when it skips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every guest-SMS path in the guidebook used the same one-shot claim to prevent double-sending, and every one of them released it on only half the failures. `sendSMS` returns `{sent:false}` ONLY for a deliberate skip — SMS_ENABLED off, the daily nudge budget, demo-org suppression — because dispatchToTelnyx throws on a timeout or any non-2xx. So the branch every site had written handled the case that isn't a failure, and a real failure walked out past the release with the claim still held. The Inngest retry then matched zero rows on `.is('', null)`, returned `already_sent`, and reported success for a message nobody ever received. For guidebook-guest-opted-in that is a guest who never gets their door code. The comment above its claim already describes this exact failure for a different case; it was still open one layer down. The same file was inverted in the other direction too: it THREW on a deliberate skip, so with SMS_ENABLED=false (production's current state) every guest opt-in produced a failing, retried run reading "SMS send failed" — untrue, and noise that would mask real failures once SMS is switched on. A retry cannot change an env var. - guidebook-guest-opted-in: release + rethrow on a throw; release + return `skipped` on a deliberate skip. - guidebook-stay-extension-handler: same for both the guest and PM-SMS claims, via a shared releaseSendClaim() that also binds the release's own result — a failed release left the claim held forever with nothing logged. - lib/sms/optin-claim: new sendClaimedDailySms() owns claim -> send -> release-on-either-exit, so the three nudge call sites (two morning, one evening) cannot drift apart again. Rendering moves inside the claimed section: renderSmsBody can throw too, and releasing only around sendSMS burned the day's slot on a template failure just the same. Also closes the silent-read family across the same feature (9 sites, both files now at 0 in the supabase-error-handling ratchet): - stay-extension-handler's context read discarded both errors, so `booking` came back null, `portalUrl` was null, and the ENTIRE guest-SMS block was skipped while the PM email went out reading "checks out on undefined" — the step reported success either way, so Inngest never retried. - stay-extension-cron: a failed bookings read looked like "this org has no checkouts", a failed existence check looked like "not yet handled", and a failed next-booking read looked like "open calendar" — each ending in a successful `dispatched: 0`. The insert now distinguishes 23505 (another run won the UNIQUE(booking_id) race — benign) from every other error, which previously took the same silent `continue`. - Both claim UPDATEs and the PM-email stamp gain `.eq('org_id', ...)`. Tests: the opt-in test that fed a `{sent:false}` and asserted a throw encoded the old inverted behaviour; split into the two cases the fix distinguishes. New unit/sms/optin-claim.test.ts covers the real helper directly, since the cron tests stub it. Every fix canary-verified by reverting it individually. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- .semgrep/baseline-counts.json | 4 +- .../functions/guidebook-guest-opted-in.ts | 38 +++- .../functions/guidebook-sms-evening-cron.ts | 30 +-- .../functions/guidebook-sms-morning-cron.ts | 68 ++++--- .../guidebook-stay-extension-cron.ts | 63 ++++++- .../guidebook-stay-extension-handler.ts | 172 +++++++++++++++--- lib/sms/optin-claim.ts | 43 +++++ unit/guardrails/n-plus-one-loops.test.ts | 2 +- .../supabase-error-handling.test.ts | 10 +- unit/inngest/guidebook-guest-opted-in.test.ts | 56 +++++- .../guidebook-sms-evening-cron.test.ts | 40 +++- .../guidebook-sms-morning-cron.test.ts | 40 +++- .../guidebook-stay-extension-handler.test.ts | 122 +++++++++++++ unit/sms/optin-claim.test.ts | 158 ++++++++++++++++ 14 files changed, 743 insertions(+), 103 deletions(-) create mode 100644 unit/sms/optin-claim.test.ts diff --git a/.semgrep/baseline-counts.json b/.semgrep/baseline-counts.json index 1bffd01b..a901ecde 100644 --- a/.semgrep/baseline-counts.json +++ b/.semgrep/baseline-counts.json @@ -99,8 +99,8 @@ ], "measured_at": "2026-08-07", "counts": { - "fieldstay-supabase-discarded-result": 83, - "fieldstay-supabase-read-without-error": 168, + "fieldstay-supabase-discarded-result": 80, + "fieldstay-supabase-read-without-error": 158, "fieldstay-supabase-read-without-error-fan-in": 0, "fieldstay-supabase-unbounded-select-in-list": 34, "fieldstay-supabase-unbounded-select-org-scoped": 98, diff --git a/lib/inngest/functions/guidebook-guest-opted-in.ts b/lib/inngest/functions/guidebook-guest-opted-in.ts index d55f980b..7e805222 100644 --- a/lib/inngest/functions/guidebook-guest-opted-in.ts +++ b/lib/inngest/functions/guidebook-guest-opted-in.ts @@ -86,10 +86,9 @@ export const guidebookGuestOptedIn = inngest.createFunction( door_code: doorCode, portal_url: portalUrl, }) - const result = await sendSMS(phoneE164, body, { orgId: property.org_id }) - - if (!result.sent) { - // SMS failed — roll back the claim so a retry can attempt again + // Releases the one-shot claim so a later run can send. Both exits below + // need it, for opposite reasons. + const releaseClaim = async (): Promise => { const { error: rollbackError } = await supabase .from('guidebook_guest_sms_optins') .update({ door_code_sent_at: null }) @@ -102,8 +101,37 @@ export const guidebookGuestOptedIn = inngest.createFunction( orgId: property.org_id, }) } + } - throw new Error(`SMS send failed: ${result.reason ?? 'unknown'}`) + // sendSMS THROWS on a real send failure — dispatchToTelnyx throws on a + // timeout or any non-2xx. That throw used to escape this step with the + // claim still held, so the Inngest retry hit + // `.is('door_code_sent_at', null)`, matched zero rows, and returned + // `already_sent` — reporting success for a door code the guest never + // received, and never trying again. That is the exact failure the + // comment above the claim describes for a different case; this was the + // same shape one layer down, still open. + let result + try { + result = await sendSMS(phoneE164, body, { orgId: property.org_id }) + } catch (err) { + await releaseClaim() + throw err + } + + if (!result.sent) { + // NOT a failure. sendSMS only returns sent:false for a DELIBERATE + // skip — SMS_ENABLED off, the daily nudge budget, demo-org + // suppression — because every real failure throws (above). Throwing + // here turned a config state into a retried failure: with + // SMS_ENABLED=false, every guest opt-in produced a failing run + // reading "SMS send failed", which is both untrue and noise that + // would mask the real failures once SMS is switched on. + // + // Release the claim so the send can happen once the skip no longer + // applies, and end cleanly — a retry cannot change an env var. + await releaseClaim() + return { skipped: result.reason ?? 'not_sent' } } return { sent: true } diff --git a/lib/inngest/functions/guidebook-sms-evening-cron.ts b/lib/inngest/functions/guidebook-sms-evening-cron.ts index 55ee777b..7760bc0a 100644 --- a/lib/inngest/functions/guidebook-sms-evening-cron.ts +++ b/lib/inngest/functions/guidebook-sms-evening-cron.ts @@ -5,7 +5,7 @@ import { createServiceClient } from '@/lib/supabase/server' import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' -import { claimDailySmsSlot, releaseDailySmsSlot } from '@/lib/sms/optin-claim' +import { sendClaimedDailySms } from '@/lib/sms/optin-claim' import { pickNearestSponsor } from '@/lib/sms/pick-nearest-sponsor' import { unwrapJoin } from '@/lib/utils/supabase-joins' import { getFeaturedAmenityLine } from '@/lib/guidebook/featured-amenities' @@ -174,20 +174,20 @@ export const guidebookSmsEveningSend = inngest.createFunction( // Claim the slot atomically before sending — a retry of this step // after a successful send now finds the slot already claimed and // skips re-sending, instead of double-texting the guest. - const claimed = await claimDailySmsSlot(supabase, optinId, 'last_evening_sms_date', todayDate) - if (!claimed) return false - - const templateKey = isRainy && primaryPool.length > 0 ? 'rain_alert' as const : 'evening_nudge' as const - const eveningBody = await renderSmsBody(orgId, templateKey, { - property_name: property.name, - offer_line: offerLine, - }) - const res = await sendSMS(optin.phone_e164, eveningBody, { category: 'nudge', orgId }) - - if (!res.sent) { - await releaseDailySmsSlot(supabase, optinId, 'last_evening_sms_date') - } - return res.sent + // Rendering is INSIDE the claimed section: it can throw too, and + // releasing only around sendSMS left the day's slot claimed for a + // template failure just the same. + return await sendClaimedDailySms( + supabase, optinId, 'last_evening_sms_date', todayDate, + async () => { + const templateKey = isRainy && primaryPool.length > 0 ? 'rain_alert' as const : 'evening_nudge' as const + const eveningBody = await renderSmsBody(orgId, templateKey, { + property_name: property.name, + offer_line: offerLine, + }) + return await sendSMS(optin.phone_e164, eveningBody, { category: 'nudge', orgId }) + }, + ) }) return { optinId, sent } diff --git a/lib/inngest/functions/guidebook-sms-morning-cron.ts b/lib/inngest/functions/guidebook-sms-morning-cron.ts index 5b4ee98d..796732dc 100644 --- a/lib/inngest/functions/guidebook-sms-morning-cron.ts +++ b/lib/inngest/functions/guidebook-sms-morning-cron.ts @@ -5,7 +5,7 @@ import { createServiceClient } from '@/lib/supabase/server' import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' -import { claimDailySmsSlot, releaseDailySmsSlot } from '@/lib/sms/optin-claim' +import { sendClaimedDailySms } from '@/lib/sms/optin-claim' import { pickNearestSponsor } from '@/lib/sms/pick-nearest-sponsor' import { unwrapJoin } from '@/lib/utils/supabase-joins' import { getFeaturedAmenityLine } from '@/lib/guidebook/featured-amenities' @@ -175,27 +175,28 @@ export const guidebookSmsMorningSend = inngest.createFunction( // Claim the slot atomically before sending — a retry of this // step after a successful send now finds the slot already // claimed and skips re-sending, instead of double-texting. - const claimed = await claimDailySmsSlot(supabase, optinId, 'last_morning_sms_date', todayDate) - if (!claimed) return false - - const rainOfferLine = buildSponsorLine( - rainySponsor.business_name, - rainySponsor.offer_type, - rainySponsor.offer_value, - rainySponsor.offer_item, - rainySponsor.custom_offer_text, - rainyDistanceMi + // Rendering is INSIDE the claimed section: it can throw too, and + // releasing only around sendSMS left the day's slot claimed for a + // template failure just the same. + return await sendClaimedDailySms( + supabase, optinId, 'last_morning_sms_date', todayDate, + async () => { + const rainOfferLine = buildSponsorLine( + rainySponsor.business_name, + rainySponsor.offer_type, + rainySponsor.offer_value, + rainySponsor.offer_item, + rainySponsor.custom_offer_text, + rainyDistanceMi + ) + + const rainBody = await renderSmsBody(orgId, 'rain_alert', { + property_name: property.name, + offer_line: rainOfferLine, + }) + return await sendSMS(optin.phone_e164, rainBody, { category: 'nudge', orgId }) + }, ) - - const rainBody = await renderSmsBody(orgId, 'rain_alert', { - property_name: property.name, - offer_line: rainOfferLine, - }) - const res = await sendSMS(optin.phone_e164, rainBody, { category: 'nudge', orgId }) - if (!res.sent) { - await releaseDailySmsSlot(supabase, optinId, 'last_morning_sms_date') - } - return res.sent } } @@ -229,20 +230,17 @@ export const guidebookSmsMorningSend = inngest.createFunction( if (!offerLine) return false // Claim the slot atomically before sending — see rain-alert branch above. - const claimed = await claimDailySmsSlot(supabase, optinId, 'last_morning_sms_date', todayDate) - if (!claimed) return false - - const morningBody = await renderSmsBody(orgId, 'morning_nudge', { - property_name: property.name, - temperature: Math.round(weather.temperature), - offer_line: offerLine, - }) - const res = await sendSMS(optin.phone_e164, morningBody, { category: 'nudge', orgId }) - - if (!res.sent) { - await releaseDailySmsSlot(supabase, optinId, 'last_morning_sms_date') - } - return res.sent + return await sendClaimedDailySms( + supabase, optinId, 'last_morning_sms_date', todayDate, + async () => { + const morningBody = await renderSmsBody(orgId, 'morning_nudge', { + property_name: property.name, + temperature: Math.round(weather.temperature), + offer_line: offerLine, + }) + return await sendSMS(optin.phone_e164, morningBody, { category: 'nudge', orgId }) + }, + ) }) return { optinId, sent } diff --git a/lib/inngest/functions/guidebook-stay-extension-cron.ts b/lib/inngest/functions/guidebook-stay-extension-cron.ts index dccf9908..34170a2c 100644 --- a/lib/inngest/functions/guidebook-stay-extension-cron.ts +++ b/lib/inngest/functions/guidebook-stay-extension-cron.ts @@ -1,6 +1,7 @@ import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap, unwrapList } from '@/lib/supabase/unwrap' const FALLBACK_TIMEZONE = 'America/New_York' @@ -62,7 +63,13 @@ export const guidebookStayExtensionCron = inngest.createFunction( Date.now() + config.extension_message_days_before * 24 * 60 * 60 * 1000 ).toISOString().split('T')[0] - const { data: bookings } = await supabase + // Unwrapped, not destructured. Discarding this error made a failed + // read indistinguishable from "this org has no checkouts that day": + // `bookings` came back null, `?? []` turned it into zero iterations, + // and the cron returned a successful `dispatched: 0`. Bounded by one + // org's checkouts on one exact date, so no pagination is needed — the + // error handling is the whole point here. + const bookingsRes = await supabase .from('bookings') .select('id, org_id, property_id, checkout_date') .eq('org_id', config.org_id) @@ -70,20 +77,35 @@ export const guidebookStayExtensionCron = inngest.createFunction( .eq('status', 'confirmed') .eq('is_block', false) + const bookings = unwrapList(bookingsRes, { + site: 'inngest.guidebook-stay-extension-cron.bookings', + orgId: config.org_id, + }) + let sent = 0 - for (const booking of bookings ?? []) { + for (const booking of bookings) { // Check if extension request already sent (idempotency via UNIQUE(booking_id)) - const { data: existing } = await supabase + const existingRes = await supabase .from('stay_extension_requests') .select('id') .eq('booking_id', booking.id) + .eq('org_id', config.org_id) .maybeSingle() + // A failed read here returns null too, which read as "not yet + // handled" — the opposite of safe. It would fall through to the + // insert and rely on UNIQUE(booking_id) to catch the duplicate, + // whose own error was then also discarded. + const existing = unwrap(existingRes, { + site: 'inngest.guidebook-stay-extension-cron.existing-request', + orgId: config.org_id, + }) + if (existing) continue // already handled // Find the NEXT booking at this property after checkout - const { data: nextBooking } = await supabase + const nextBookingRes = await supabase .from('bookings') .select('id, checkin_date') .eq('property_id', booking.property_id) @@ -94,6 +116,14 @@ export const guidebookStayExtensionCron = inngest.createFunction( .limit(1) .maybeSingle() + // Same shape again: a failed read looked identical to "no future + // booking", which `continue`s — silently declining to offer the + // extension rather than retrying. + const nextBooking = unwrap(nextBookingRes, { + site: 'inngest.guidebook-stay-extension-cron.next-booking', + orgId: config.org_id, + }) + // Calculate gap const nextCheckin = nextBooking?.checkin_date if (!nextCheckin) continue // no future booking = open calendar, don't offer @@ -107,14 +137,23 @@ export const guidebookStayExtensionCron = inngest.createFunction( if (gapDays < config.extension_gap_threshold_days) continue // gap too small // Get guest SMS opt-in if available - const { data: optin } = await supabase + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('phone_e164, is_active') .eq('booking_id', booking.id) .maybeSingle() + // A dropped error here degrades silently rather than failing: the + // request is still created and the PM still notified, but + // guestPhoneE164 goes out null, so the guest half of the gap-night + // offer is never sent and nothing says why. + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-stay-extension-cron.optin', + orgId: config.org_id, + }) + // Create the extension request record - const { data: request } = await supabase + const requestRes = await supabase .from('stay_extension_requests') .insert({ org_id: config.org_id, @@ -128,6 +167,18 @@ export const guidebookStayExtensionCron = inngest.createFunction( .select('id') .single() + // 23505 is the ONE benign outcome: another run won the race against + // UNIQUE(booking_id) between the existence check above and here, so + // that run owns the notification. Every other error — an FK + // violation, a constraint failure, an outage — used to take the same + // silent `continue`, dropping the offer and still reporting success. + if (requestRes.error?.code === '23505') continue + + const request = unwrap(requestRes, { + site: 'inngest.guidebook-stay-extension-cron.insert-request', + orgId: config.org_id, + }) + if (!request) continue // Fire event to handle notification + SMS diff --git a/lib/inngest/functions/guidebook-stay-extension-handler.ts b/lib/inngest/functions/guidebook-stay-extension-handler.ts index ec0e2150..a02cf79a 100644 --- a/lib/inngest/functions/guidebook-stay-extension-handler.ts +++ b/lib/inngest/functions/guidebook-stay-extension-handler.ts @@ -1,8 +1,64 @@ import { inngest } from '@/lib/inngest/client' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' import { sendSMS, normalizePhoneToE164 } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' import { getPmEmails, getPmMembers } from '@/lib/inngest/helpers' +import { reportError } from '@/lib/observability/report-error' + +/** Minimal shape of the service client, enough for the claim-release helper. */ +type ClaimClient = ReturnType + +/** + * Releases a one-shot send claim (`sms_sent_at` / `pm_notified_at`) so a later + * run can try again. + * + * Both call sites need this for two OPPOSITE reasons, and the second one was + * missing entirely: + * + * - `sendSMS` returns `{sent:false}` only for a DELIBERATE skip — SMS_ENABLED + * off, the daily nudge budget, demo-org suppression. Release so the send can + * happen once the skip no longer applies, then end cleanly; a retry cannot + * change an env var. + * - `sendSMS` THROWS on a real failure (dispatchToTelnyx throws on a timeout + * or any non-2xx). That throw escaped the step with the claim still held, so + * the Inngest retry hit `.is('', null)`, matched zero rows, and + * returned `already_sent` — reporting success for a message nobody ever + * received, and never trying again. + * + * The release's own result was also discarded. A failed release leaves the + * claim held forever with no signal anywhere, which is the same silent dead end + * by a slower route. + */ +async function releaseSendClaim( + supabase: ClaimClient, + requestId: string, + column: 'sms_sent_at' | 'pm_notified_at', + orgId: string, +): Promise { + // Spelled out rather than computed: a `{ [column]: null }` payload widens to + // an index signature the generated table types reject, and casting past that + // would also cast away the protection against naming a column that isn't + // there. + const patch = column === 'sms_sent_at' + ? { sms_sent_at: null } + : { pm_notified_at: null } + + const { error } = await supabase + .from('stay_extension_requests') + .update(patch) + .eq('id', requestId) + .eq('org_id', orgId) + + if (error) { + console.error(`[guidebook-stay-extension-handler] ${column} claim release failed`, error.message) + reportError(error, { + site: 'inngest.guidebook-stay-extension-handler.claim-release', + orgId, + extra: { column }, + }) + } +} export const guidebookStayExtensionHandler = inngest.createFunction( { id: 'guidebook-stay-extension-handler', name: 'Guidebook: Stay Extension Notify' }, @@ -14,7 +70,15 @@ export const guidebookStayExtensionHandler = inngest.createFunction( guestPhoneE164, contactMethod, } = event.data - // Fetch property and booking context + // Fetch property and booking context. + // + // Both errors used to be discarded. A failed read left `booking` null, so + // `portalUrl` was null, so the ENTIRE guest-SMS block below was skipped + // silently — the gap-night offer this whole function exists to send was + // never sent — while the PM email went out reading "checks out on + // undefined". The step returned successfully either way, so Inngest never + // retried it and nothing was logged. Throwing is what makes the retry + // happen. const { property, booking } = await step.run('fetch-context', async () => { const supabase = createServiceClient({ system: 'inngest:guidebook-stay-extension-handler' }) const [propRes, bookRes] = await Promise.all([ @@ -22,14 +86,19 @@ export const guidebookStayExtensionHandler = inngest.createFunction( .from('properties') .select('name') .eq('id', propertyId) + .eq('org_id', orgId) .single(), supabase .from('bookings') .select('guidebook_token, checkout_date') .eq('id', bookingId) + .eq('org_id', orgId) .single(), ]) - return { property: propRes.data, booking: bookRes.data } + return { + property: unwrap(propRes, { site: 'inngest.guidebook-stay-extension-handler.property', orgId }), + booking: unwrap(bookRes, { site: 'inngest.guidebook-stay-extension-handler.booking', orgId }), + } }) const appUrl = process.env.NEXT_PUBLIC_APP_URL ?? 'https://app.fieldstay.app' @@ -47,27 +116,44 @@ export const guidebookStayExtensionHandler = inngest.createFunction( // can execute an arbitrary amount of time later, wide enough for a // guest to have texted STOP in between. Every other guest SMS path // re-checks immediately before sending; this one didn't. - const { data: optin } = await supabase + // + // Unwrapped, not destructured: a failed consent read must NOT be read + // as "no consent". Both readings skip the send, but only one of them + // should do so silently and permanently — the other needs a retry. + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('is_active') .eq('booking_id', bookingId) .maybeSingle() - if (!optin?.is_active) return + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-stay-extension-handler.optin-recheck', + orgId, + }) + + if (!optin?.is_active) return { skipped: 'not_opted_in' } // ── Atomic claim — wins the race, prevents double-send on retry ─────── // UPDATE only succeeds if sms_sent_at IS NULL. If this step is retried // after a successful SMS send, the timestamp is already set, the // UPDATE affects 0 rows, and we skip the send. Mirrors the pattern in - // guidebook-guest-opted-in.ts. - const { data: claimed } = await supabase + // guidebook-guest-opted-in.ts — including the part that file learned + // and this one hadn't: a FAILED claim also returns null, which read as + // "already sent" and skipped forever. Throwing lets Inngest retry. + const claimRes = await supabase .from('stay_extension_requests') .update({ sms_sent_at: new Date().toISOString() }) .eq('id', requestId) + .eq('org_id', orgId) .is('sms_sent_at', null) .select('id') .maybeSingle() + const claimed = unwrap(claimRes, { + site: 'inngest.guidebook-stay-extension-handler.guest-sms-claim', + orgId, + }) + if (!claimed) return { skipped: 'already_sent' } const discountLine = discountPct @@ -83,15 +169,21 @@ export const guidebookStayExtensionHandler = inngest.createFunction( // 'nudge': guest marketing message — counts against the platform-wide // daily SMS budget (the PM notification below is operational and doesn't) - const result = await sendSMS(guestPhoneE164, text, { category: 'nudge', orgId }) + // See releaseSendClaim() for why both exits below release the claim. + let result + try { + result = await sendSMS(guestPhoneE164, text, { category: 'nudge', orgId }) + } catch (err) { + await releaseSendClaim(supabase, requestId, 'sms_sent_at', orgId) + throw err + } if (!result.sent) { - // SMS failed — roll back the claim so a retry can attempt again - await supabase - .from('stay_extension_requests') - .update({ sms_sent_at: null }) - .eq('id', requestId) + await releaseSendClaim(supabase, requestId, 'sms_sent_at', orgId) + return { skipped: result.reason ?? 'not_sent' } } + + return { sent: true } }) } @@ -110,11 +202,19 @@ export const guidebookStayExtensionHandler = inngest.createFunction( const [pmMember] = await getPmMembers(supabase, orgId, { limit: 1 }) if (!pmMember) return { skipped: true } - const { data: profile } = await supabase + // maybeSingle, not single: a PM with no profile row is a legitimate + // skip, and .single() turns that into a PGRST116 error that unwrap + // would now escalate into a retried failure. + const profileRes = await supabase .from('profiles') .select('phone') .eq('id', pmMember.userId) - .single() + .maybeSingle() + + const profile = unwrap(profileRes, { + site: 'inngest.guidebook-stay-extension-handler.pm-profile', + orgId, + }) if (!profile?.phone) return { skipped: true, reason: 'no_pm_phone' } @@ -126,14 +226,23 @@ export const guidebookStayExtensionHandler = inngest.createFunction( // caller that wins the UPDATE sends. A retry after a successful // send finds pm_notified_at already set and skips, instead of // texting the PM's own phone twice. - const { data: claimed } = await supabase + // Unwrapped for the same reason as the guest claim above: a failed + // claim also returns null, and reading that as "already notified" + // drops the notification permanently. + const claimRes = await supabase .from('stay_extension_requests') .update({ pm_notified_at: new Date().toISOString() }) .eq('id', requestId) + .eq('org_id', orgId) .is('pm_notified_at', null) .select('id') .maybeSingle() + const claimed = unwrap(claimRes, { + site: 'inngest.guidebook-stay-extension-handler.pm-sms-claim', + orgId, + }) + if (!claimed) return { skipped: true, reason: 'already_notified' } const text = @@ -141,17 +250,21 @@ export const guidebookStayExtensionHandler = inngest.createFunction( `${booking?.checkout_date}, ${gapDays} day${gapDays !== 1 ? 's' : ''} ` + `before next booking.${discountLine} Guest was messaged via the guidebook.` - const result = await sendSMS(e164, text, { orgId }) + // See releaseSendClaim() — same two exits as the guest send above. + let result + try { + result = await sendSMS(e164, text, { orgId }) + } catch (err) { + await releaseSendClaim(supabase, requestId, 'pm_notified_at', orgId) + throw err + } if (!result.sent) { - // SMS failed — roll back the claim so a retry can attempt again - await supabase - .from('stay_extension_requests') - .update({ pm_notified_at: null }) - .eq('id', requestId) + await releaseSendClaim(supabase, requestId, 'pm_notified_at', orgId) + return { notified: false, skipped: result.reason ?? 'not_sent' } } - return { notified: result.sent } + return { notified: true } }) } else { // contactMethod === 'email' (also the fallback if null/unset) @@ -181,10 +294,23 @@ export const guidebookStayExtensionHandler = inngest.createFunction( ) if (error) throw new Error(`Resend error: ${JSON.stringify(error)}`) - await supabase + // No claim needed on this path — Resend's idempotencyKey is what + // prevents the double-send, so this write is only a record of it. + // Its result was discarded entirely, which meant a failed write left + // the request looking un-notified forever with nothing logged. + const { error: stampError } = await supabase .from('stay_extension_requests') .update({ pm_notified_at: new Date().toISOString() }) .eq('id', requestId) + .eq('org_id', orgId) + + if (stampError) { + console.error('[guidebook-stay-extension-handler] pm_notified_at stamp failed', stampError.message) + reportError(stampError, { + site: 'inngest.guidebook-stay-extension-handler.pm-email-stamp', + orgId, + }) + } return { notified: true } }) diff --git a/lib/sms/optin-claim.ts b/lib/sms/optin-claim.ts index 17ec7852..6f45a302 100644 --- a/lib/sms/optin-claim.ts +++ b/lib/sms/optin-claim.ts @@ -48,3 +48,46 @@ export async function releaseDailySmsSlot( reportError(error, { site: 'lib.sms.optin-claim.release', extra: { date_column: dateColumn } }) } } + +/** + * Claim → send → release-on-EITHER-failure, as one unit. + * + * The claim and the release already lived here; the sequence that ties them + * together did not, and all three guest-nudge call sites (two morning + * branches, one evening) had independently written the same HALF of it: + * + * const res = await sendSMS(...) + * if (!res.sent) await releaseDailySmsSlot(...) + * return res.sent + * + * `sendSMS` returns `{sent:false}` ONLY for a deliberate skip — SMS_ENABLED + * off, the daily nudge budget, demo-org suppression. Every REAL failure + * throws, because dispatchToTelnyx throws on a timeout or any non-2xx. So the + * one branch that was written handled the case that isn't a failure, and the + * throw walked out past the release with the day's slot still claimed. The + * Inngest retry then re-read the date column, found today's date, skipped — + * and that guest's nudge was silently gone for the day. + * + * Callers get a plain boolean back, so the shape at each site is unchanged. + */ +export async function sendClaimedDailySms( + supabase: SupabaseClient, + optinId: string, + dateColumn: DailySmsDateColumn, + todayDate: string, + send: () => Promise<{ sent: boolean }>, +): Promise { + const claimed = await claimDailySmsSlot(supabase, optinId, dateColumn, todayDate) + if (!claimed) return false + + let res: { sent: boolean } + try { + res = await send() + } catch (err) { + await releaseDailySmsSlot(supabase, optinId, dateColumn) + throw err + } + + if (!res.sent) await releaseDailySmsSlot(supabase, optinId, dateColumn) + return res.sent +} diff --git a/unit/guardrails/n-plus-one-loops.test.ts b/unit/guardrails/n-plus-one-loops.test.ts index ab87da40..f81b53bf 100644 --- a/unit/guardrails/n-plus-one-loops.test.ts +++ b/unit/guardrails/n-plus-one-loops.test.ts @@ -127,7 +127,7 @@ const EXCEPTIONS: Record = { 'Real N+1 (existence-check select + insert per property) left as a known, bounded cost — deferred rather than fixed blind in the same PR that added this guardrail, since it touches live PMS-sync logic. Bounded by properties-per-org (10-50 per CLAUDE.md\'s target user).', 'lib/asset-discovery/seed-from-amenities.ts:171': 'Second pass (absent-asset-types) of the same function — same reasoning as line 63.', - 'lib/inngest/functions/guidebook-stay-extension-cron.ts:75': + 'lib/inngest/functions/guidebook-stay-extension-cron.ts:87': 'Real N+1 (existence check, next-booking lookup, opt-in lookup, insert — 4 queries per booking) left as a known, bounded cost — deferred rather than fixed blind, touches live guest-messaging sync logic. Bounded by same-day checkouts per org per day.', 'lib/inngest/functions/ownerrez/reconciliation-handler.ts:124': 'Real N+1 (cancel booking + cancel its turnovers, per stale booking) left as a known, bounded cost — deferred rather than fixed blind. Contrast lib/inngest/functions/ical-sync.ts, which batches the equivalent booking-cancel via .update().in(\'id\', ids) — a good template for fixing this one later.', diff --git a/unit/guardrails/supabase-error-handling.test.ts b/unit/guardrails/supabase-error-handling.test.ts index 3d4a7cbc..0c17daaf 100644 --- a/unit/guardrails/supabase-error-handling.test.ts +++ b/unit/guardrails/supabase-error-handling.test.ts @@ -128,8 +128,14 @@ const BASELINE: Record = { 'lib/inngest/functions/flagged-turnover-wo.ts': 3, 'lib/inngest/functions/guidebook-sms-evening-cron.ts': 3, 'lib/inngest/functions/guidebook-sms-morning-cron.ts': 4, - 'lib/inngest/functions/guidebook-stay-extension-cron.ts': 5, - 'lib/inngest/functions/guidebook-stay-extension-handler.ts': 4, + // 5 -> 0 and 4 -> 0 (entries deleted): the gap-night offer's whole failure + // surface was silent. In the cron a failed bookings read looked like "this + // org has no checkouts", a failed existence check looked like "not yet + // handled", and a failed next-booking read looked like "open calendar" — + // each ending in a successful `dispatched: 0`. In the handler a failed + // context read left `booking` null, so `portalUrl` was null, so the guest + // SMS block was skipped entirely while the PM email went out reading + // "checks out on undefined". 'lib/inngest/functions/hospitable/hospitable-reviews-backfill.ts': 2, 'lib/inngest/functions/hospitable/incremental-sync.ts': 5, 'lib/inngest/functions/hospitable/initial-sync.ts': 2, diff --git a/unit/inngest/guidebook-guest-opted-in.test.ts b/unit/inngest/guidebook-guest-opted-in.test.ts index ceaf14cb..5d08baaa 100644 --- a/unit/inngest/guidebook-guest-opted-in.test.ts +++ b/unit/inngest/guidebook-guest-opted-in.test.ts @@ -158,26 +158,70 @@ describe('guidebookGuestOptedIn', () => { expect(result).toEqual({ optinId: 'optin_1', sentDoorCode: true }) }) - it('rolls back the claim and throws when the SMS send fails, so a retry can attempt again', async () => { + // The two exits below are NOT the same event, and conflating them is what + // shipped: sendSMS THROWS on a real send failure (dispatchToTelnyx throws on + // a timeout or any non-2xx) and returns {sent:false} only for a DELIBERATE + // skip. One test used to feed a {sent:false} and assert a throw, which + // asserted the wrong half of both behaviours at once. + + it('releases the claim and rethrows when the SMS send THROWS, so the Inngest retry can send again', async () => { const supabase = makeSupabase( { properties: [{ data: propertyRow, error: null }], bookings: [{ data: bookingRow, error: null }], guidebook_guest_sms_optins: [ { data: { id: 'optin_1' }, error: null }, // claim succeeds - { data: null, error: null }, // rollback update + { data: null, error: null }, // claim release ], }, { data: '4321', error: null } ) ;(createServiceClient as ReturnType).mockReturnValue(supabase) - ;(sendSMS as ReturnType).mockResolvedValueOnce({ sent: false, reason: 'SMS_ENABLED is not true' }) + ;(sendSMS as ReturnType).mockRejectedValueOnce(new Error('Telnyx 502')) await expect( invokeHandler(guidebookGuestOptedIn, { event: optedInEvent(), step: makeStep() }) - ).rejects.toThrow('SMS send failed: SMS_ENABLED is not true') + ).rejects.toThrow('Telnyx 502') + + // Without the release, the retry hits `.is('door_code_sent_at', null)`, + // matches zero rows, returns `already_sent` — and the guest never gets + // their door code while the run reports success. + const releaseCall = supabase.calls.filter( + (c) => c.table === 'guidebook_guest_sms_optins' && c.method === 'update' + )[1] + expect(releaseCall?.args[0]).toEqual({ door_code_sent_at: null }) + }) + + it('releases the claim and ends cleanly — no throw — when sendSMS reports a deliberate skip', async () => { + const supabase = makeSupabase( + { + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [ + { data: { id: 'optin_1' }, error: null }, // claim succeeds + { data: null, error: null }, // claim release + ], + }, + { data: '4321', error: null } + ) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(sendSMS as ReturnType).mockResolvedValueOnce({ sent: false, reason: 'SMS_ENABLED is not true' }) + + // SMS_ENABLED=false is production's current state. Throwing here made + // EVERY guest opt-in a failing, retried Inngest run reading "SMS send + // failed" — untrue, and noise that would mask real failures once SMS is + // switched on. A retry cannot change an env var. + const result = await invokeHandler(guidebookGuestOptedIn, { + event: optedInEvent(), + step: makeStep(), + }) + expect(result).toEqual({ optinId: 'optin_1', sentDoorCode: true }) - const rollbackCall = supabase.calls.filter((c) => c.table === 'guidebook_guest_sms_optins' && c.method === 'update')[1] - expect(rollbackCall?.args[0]).toEqual({ door_code_sent_at: null }) + // Still released, for the opposite reason: so the send can happen once + // the skip no longer applies. + const releaseCall = supabase.calls.filter( + (c) => c.table === 'guidebook_guest_sms_optins' && c.method === 'update' + )[1] + expect(releaseCall?.args[0]).toEqual({ door_code_sent_at: null }) }) }) diff --git a/unit/inngest/guidebook-sms-evening-cron.test.ts b/unit/inngest/guidebook-sms-evening-cron.test.ts index 80776845..79a45d4c 100644 --- a/unit/inngest/guidebook-sms-evening-cron.test.ts +++ b/unit/inngest/guidebook-sms-evening-cron.test.ts @@ -13,10 +13,42 @@ vi.mock('@/lib/sms/telnyx', () => ({ vi.mock('@/lib/sms/templates', () => ({ renderSmsBody: vi.fn(async () => 'rendered sms body'), })) -vi.mock('@/lib/sms/optin-claim', () => ({ - claimDailySmsSlot: vi.fn(async () => true), - releaseDailySmsSlot: vi.fn(async () => undefined), -})) +// sendClaimedDailySms is stubbed with a faithful delegating implementation +// rather than a bare vi.fn(): every assertion in this file is about WHICH slot +// gets claimed and whether it is released, and those calls now happen inside +// the helper. Delegating keeps them observable here. The helper's own +// release-on-throw contract is tested directly in unit/sms/optin-claim.test.ts +// — the crons must not be the only place it is covered. +vi.mock('@/lib/sms/optin-claim', () => { + const claimDailySmsSlot = vi.fn(async () => true) + const releaseDailySmsSlot = vi.fn(async () => undefined) + return { + claimDailySmsSlot, + releaseDailySmsSlot, + sendClaimedDailySms: vi.fn(async ( + supabase: unknown, + optinId: string, + dateColumn: string, + todayDate: string, + send: () => Promise<{ sent: boolean }>, + ) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const claimed = await (claimDailySmsSlot as any)(supabase, optinId, dateColumn, todayDate) + if (!claimed) return false + let res: { sent: boolean } + try { + res = await send() + } catch (err) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + throw err + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if (!res.sent) await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + return res.sent + }), + } +}) import { guidebookSmsEveningCron, guidebookSmsEveningSend } from '@/lib/inngest/functions/guidebook-sms-evening-cron' import { createServiceClient } from '@/lib/supabase/server' diff --git a/unit/inngest/guidebook-sms-morning-cron.test.ts b/unit/inngest/guidebook-sms-morning-cron.test.ts index 5fa64264..14336162 100644 --- a/unit/inngest/guidebook-sms-morning-cron.test.ts +++ b/unit/inngest/guidebook-sms-morning-cron.test.ts @@ -13,10 +13,42 @@ vi.mock('@/lib/sms/telnyx', () => ({ vi.mock('@/lib/sms/templates', () => ({ renderSmsBody: vi.fn(async () => 'rendered sms body'), })) -vi.mock('@/lib/sms/optin-claim', () => ({ - claimDailySmsSlot: vi.fn(async () => true), - releaseDailySmsSlot: vi.fn(async () => undefined), -})) +// sendClaimedDailySms is stubbed with a faithful delegating implementation +// rather than a bare vi.fn(): every assertion in this file is about WHICH slot +// gets claimed and whether it is released, and those calls now happen inside +// the helper. Delegating keeps them observable here. The helper's own +// release-on-throw contract is tested directly in unit/sms/optin-claim.test.ts +// — the crons must not be the only place it is covered. +vi.mock('@/lib/sms/optin-claim', () => { + const claimDailySmsSlot = vi.fn(async () => true) + const releaseDailySmsSlot = vi.fn(async () => undefined) + return { + claimDailySmsSlot, + releaseDailySmsSlot, + sendClaimedDailySms: vi.fn(async ( + supabase: unknown, + optinId: string, + dateColumn: string, + todayDate: string, + send: () => Promise<{ sent: boolean }>, + ) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const claimed = await (claimDailySmsSlot as any)(supabase, optinId, dateColumn, todayDate) + if (!claimed) return false + let res: { sent: boolean } + try { + res = await send() + } catch (err) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + throw err + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if (!res.sent) await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + return res.sent + }), + } +}) import { guidebookSmsMorningCron, guidebookSmsMorningSend } from '@/lib/inngest/functions/guidebook-sms-morning-cron' import { createServiceClient } from '@/lib/supabase/server' diff --git a/unit/inngest/guidebook-stay-extension-handler.test.ts b/unit/inngest/guidebook-stay-extension-handler.test.ts index e5110013..6f8ab408 100644 --- a/unit/inngest/guidebook-stay-extension-handler.test.ts +++ b/unit/inngest/guidebook-stay-extension-handler.test.ts @@ -229,6 +229,128 @@ describe('guidebookStayExtensionHandler', () => { expect(sendSMS).not.toHaveBeenCalled() }) + it('throws instead of silently dropping the offer when the context read fails', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: null, error: { message: 'connection reset', code: '08006' } }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getPmEmails as ReturnType).mockResolvedValue(['pm@example.com']) + + // Both errors used to be discarded, which left `booking` null — so + // `portalUrl` was null, the entire guest-SMS block was skipped, and the PM + // email went out reading "checks out on undefined". The step reported + // success, so Inngest never retried and nothing was logged. + await expect( + invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + ).rejects.toThrow(/Supabase query failed/) + + expect(sendSMS).not.toHaveBeenCalled() + expect(resend.emails.send).not.toHaveBeenCalled() + }) + + // The two guest-SMS exits below are NOT the same event. sendSMS THROWS on a + // real send failure (dispatchToTelnyx throws on a timeout or any non-2xx) and + // returns {sent:false} only for a DELIBERATE skip. Only the {sent:false} half + // released the claim; the throw escaped with it still held. + it('releases the guest-SMS claim and rethrows when the send THROWS, so the retry can send again', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [{ data: { is_active: true }, error: null }], + stay_extension_requests: [ + { data: { id: 'req_1' }, error: null }, // claim succeeds + { data: null, error: null }, // claim release + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(sendSMS as ReturnType).mockRejectedValueOnce(new Error('Telnyx 502')) + + await expect( + invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + ).rejects.toThrow('Telnyx 502') + + // Without the release, the retry hits `.is('sms_sent_at', null)`, matches + // zero rows, returns `already_sent` — the guest never gets the offer and + // the run reports success. + const releaseCall = supabase.calls.filter( + (c) => c.table === 'stay_extension_requests' && c.method === 'update', + )[1] + expect(releaseCall?.args[0]).toEqual({ sms_sent_at: null }) + }) + + it('releases the guest-SMS claim and ends cleanly — no throw — when the send is a deliberate skip', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [{ data: { is_active: true }, error: null }], + stay_extension_requests: [ + { data: { id: 'req_1' }, error: null }, // claim succeeds + { data: null, error: null }, // claim release + { data: null, error: null }, // pm_notified_at stamp + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getPmEmails as ReturnType).mockResolvedValue(['pm@example.com']) + ;(sendSMS as ReturnType).mockResolvedValueOnce({ sent: false, reason: 'SMS_ENABLED is not true' }) + + // SMS_ENABLED=false is production's current state — it must not turn every + // gap-night offer into a retried failure. Released anyway, for the opposite + // reason: so the send can happen once the skip no longer applies. + await invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + + const releaseCall = supabase.calls.filter( + (c) => c.table === 'stay_extension_requests' && c.method === 'update', + )[1] + expect(releaseCall?.args[0]).toEqual({ sms_sent_at: null }) + expect(resend.emails.send).toHaveBeenCalled() + }) + + it('releases the PM-SMS claim and rethrows when the PM send THROWS', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + profiles: [{ data: { phone: '512-555-9999' }, error: null }], + stay_extension_requests: [ + { data: { id: 'req_1' }, error: null }, // pm-sms claim succeeds + { data: null, error: null }, // claim release + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getPmMembers as ReturnType).mockResolvedValue([{ userId: 'u1', email: 'pm@example.com', role: 'owner' }]) + ;(sendSMS as ReturnType).mockRejectedValueOnce(new Error('Telnyx 502')) + + await expect( + invokeHandler(guidebookStayExtensionHandler, { + event: requestEvent({ guestPhoneE164: null, contactMethod: 'sms' }), + step: makeStep(), + }) + ).rejects.toThrow('Telnyx 502') + + const releaseCall = supabase.calls.filter( + (c) => c.table === 'stay_extension_requests' && c.method === 'update', + )[1] + expect(releaseCall?.args[0]).toEqual({ pm_notified_at: null }) + }) + + it('retries rather than skipping forever when the atomic claim itself errors', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [{ data: { is_active: true }, error: null }], + // A FAILED claim returns null data too — indistinguishable from "already + // claimed" once the error is dropped, so the send was skipped forever. + stay_extension_requests: [{ data: null, error: { message: 'deadlock detected', code: '40P01' } }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + await expect( + invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + ).rejects.toThrow(/Supabase query failed/) + + expect(sendSMS).not.toHaveBeenCalled() + }) + it('idempotency: does not re-notify the PM by SMS when a prior run already claimed pm_notified_at', async () => { const supabase = makeSupabase({ properties: [{ data: propertyRow, error: null }], diff --git a/unit/sms/optin-claim.test.ts b/unit/sms/optin-claim.test.ts new file mode 100644 index 00000000..5175f721 --- /dev/null +++ b/unit/sms/optin-claim.test.ts @@ -0,0 +1,158 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('@/lib/observability/report-error', () => ({ + reportError: vi.fn(), +})) + +import { + claimDailySmsSlot, + releaseDailySmsSlot, + sendClaimedDailySms, +} from '@/lib/sms/optin-claim' +import type { SupabaseClient } from '@supabase/supabase-js' + +/** + * Minimal `.from(table)` chain. Every terminal (`maybeSingle`, or awaiting the + * builder directly) consumes the next queued result in call order, so the claim + * UPDATE and the release UPDATE can be scripted independently. + */ +function makeSupabase(queued: { data?: unknown; error?: unknown }[]) { + let idx = 0 + const calls: { method: string; args: unknown[] }[] = [] + + const from = vi.fn(() => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const chain: any = {} + const record = (method: string, args: unknown[]) => { + calls.push({ method, args }) + return chain + } + chain.update = (...a: unknown[]) => record('update', a) + chain.eq = (...a: unknown[]) => record('eq', a) + chain.or = (...a: unknown[]) => record('or', a) + chain.select = (...a: unknown[]) => record('select', a) + + const resolveNext = () => Promise.resolve(queued[idx++] ?? { data: null, error: null }) + chain.maybeSingle = () => resolveNext() + chain.then = (resolve: (v: unknown) => unknown, reject?: (e: unknown) => unknown) => + resolveNext().then(resolve, reject) + return chain + }) + + return { client: { from } as unknown as SupabaseClient, calls } +} + +const CLAIM_WON = { data: { id: 'optin_1' }, error: null } +const CLAIM_LOST = { data: null, error: null } +const NO_ROWS = { data: null, error: null } + +describe('claimDailySmsSlot', () => { + beforeEach(() => vi.clearAllMocks()) + + it('returns true when this call won the atomic claim', async () => { + const { client } = makeSupabase([CLAIM_WON]) + await expect( + claimDailySmsSlot(client, 'optin_1', 'last_morning_sms_date', '2026-08-07') + ).resolves.toBe(true) + }) + + it('returns false — not an error — when a prior run already claimed today', async () => { + const { client } = makeSupabase([CLAIM_LOST]) + await expect( + claimDailySmsSlot(client, 'optin_1', 'last_morning_sms_date', '2026-08-07') + ).resolves.toBe(false) + }) + + it('throws when the claim query itself fails, instead of reading it as already-sent', async () => { + // A failed claim returns null data too. Reporting that as false told every + // caller "already sent today" and suppressed the guest's SMS silently. + const { client } = makeSupabase([{ data: null, error: { message: 'deadlock detected', code: '40P01' } }]) + await expect( + claimDailySmsSlot(client, 'optin_1', 'last_morning_sms_date', '2026-08-07') + ).rejects.toThrow(/Supabase query failed/) + }) +}) + +describe('sendClaimedDailySms', () => { + beforeEach(() => vi.clearAllMocks()) + + it('sends and keeps the claim on success', async () => { + const { client, calls } = makeSupabase([CLAIM_WON]) + const send = vi.fn(async () => ({ sent: true })) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_morning_sms_date', '2026-08-07', send) + ).resolves.toBe(true) + + expect(send).toHaveBeenCalled() + // Exactly one UPDATE: the claim. No release. + expect(calls.filter((c) => c.method === 'update')).toHaveLength(1) + }) + + it('never calls send when the claim was already taken', async () => { + const { client } = makeSupabase([CLAIM_LOST]) + const send = vi.fn(async () => ({ sent: true })) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_evening_sms_date', '2026-08-07', send) + ).resolves.toBe(false) + expect(send).not.toHaveBeenCalled() + }) + + // The two failure exits below are NOT the same event, and only one of them + // was handled at the three call sites this helper replaced. sendSMS returns + // {sent:false} ONLY for a deliberate skip (SMS_ENABLED off, nudge budget, + // demo suppression); every real failure THROWS out of dispatchToTelnyx. + it('releases the slot and rethrows when the send throws, so the day is not burned', async () => { + const { client, calls } = makeSupabase([CLAIM_WON, NO_ROWS]) + const send = vi.fn(async () => { throw new Error('Telnyx 502') }) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_morning_sms_date', '2026-08-07', send) + ).rejects.toThrow('Telnyx 502') + + // Without the release the Inngest retry re-reads the date column, finds + // today's date, skips — and that guest's nudge is gone for the day. + const updates = calls.filter((c) => c.method === 'update') + expect(updates).toHaveLength(2) + expect(updates[1].args[0]).toEqual({ last_morning_sms_date: null }) + }) + + it('releases the slot and returns false — without throwing — on a deliberate skip', async () => { + const { client, calls } = makeSupabase([CLAIM_WON, NO_ROWS]) + const send = vi.fn(async () => ({ sent: false })) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_evening_sms_date', '2026-08-07', send) + ).resolves.toBe(false) + + const updates = calls.filter((c) => c.method === 'update') + expect(updates).toHaveLength(2) + expect(updates[1].args[0]).toEqual({ last_evening_sms_date: null }) + }) + + it('covers rendering too — a template failure inside send releases the slot', async () => { + // Rendering used to sit BETWEEN the claim and the try-less send, so a + // renderSmsBody throw burned the day's slot with no release at all. + const { client, calls } = makeSupabase([CLAIM_WON, NO_ROWS]) + const send = vi.fn(async () => { throw new Error('unknown template key') }) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_morning_sms_date', '2026-08-07', send) + ).rejects.toThrow('unknown template key') + expect(calls.filter((c) => c.method === 'update')).toHaveLength(2) + }) +}) + +describe('releaseDailySmsSlot', () => { + beforeEach(() => vi.clearAllMocks()) + + it('swallows a failed release rather than masking the original error', async () => { + // Called from a catch block on the throw path — rethrowing here would + // replace the real send failure with a rollback failure. + const { client } = makeSupabase([{ data: null, error: { message: 'timeout', code: '57014' } }]) + await expect( + releaseDailySmsSlot(client, 'optin_1', 'last_morning_sms_date') + ).resolves.toBeUndefined() + }) +}) From adbf0b89afa1480cdd56c9b26e925b8ab11a4d51 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 15:30:48 +0000 Subject: [PATCH 04/11] fix(guidebook): stop the sponsor checkout from minting duplicate subscriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `guidebook_sponsors.checkout_session_id` was written on every checkout and then never read back from anywhere in the codebase. The field exists for exactly one purpose — the work-order invoice route has used it that way all along ("Store the session ID for potential reuse on duplicate clicks", app/api/invoices/[invoiceId]/checkout/route.ts) — but the sponsor path only ever wrote it. So every click, reload, or retry minted a fresh Stripe Checkout Session, each payable for 24 hours. Two of them paid means two subscriptions, two checkout.session.completed webhooks with DISTINCT event ids (so stripe_processed_events does not collapse them), and an activation handler that overwrites stripe_subscription_id with whichever lands last — leaving the other subscription billing the business monthly with nothing in FieldStay able to reach it. - Reuse an already-open session instead of creating a second one. - Compare-and-swap the new session id against the value just read, rather than blind-overwriting. The reuse check covers the repeat-click case; the CAS covers the concurrent one. The loser expires its own session and returns the winner's, so exactly one payable session exists per sponsor. A plain check-then-write is the TOCTOU the audit checklist calls out — the precondition belongs in the WHERE clause. - Refuse checkout from `payment_failed`, not just `active`. payment_failed is set from invoice.payment_failed, which does NOT end the subscription: Stripe keeps dunning and guidebook-sponsor-payment-recovered flips the row straight back to active. A sponsor who saw the failure notice could buy a second subscription while the first was still being retried. `cancelled` stays allowed — that subscription is genuinely gone. - Bind the CAS result. With reuse in place this write is load-bearing: a silent failure degrades straight back to "new session every time". Also in the activation handler: - Report when a second, DIFFERENT subscription id replaces a live one. Our own flow should no longer produce this, but a subscription created outside it still can, and orphaning one must not pass unnoticed. The write proceeds. - Throw on the guidebook-unlock upsert, matching the identical upsert in guidebook-sponsor-payment-recovered. Discarded, a failed unlock left the guidebook locked while the step returned `wasUnlocked: true` and the audit row recorded an unlock that never happened. And the media-kit-token lookup now unwraps rather than discarding its error — a sponsor holding a valid link was told it was invalid whenever the query itself failed. Byte-for-byte the defect already fixed in optInGuestSms, one function over in the same file. Verified against production first: 2 sponsors, both `pending`, zero checkouts ever completed — so no live billing was affected. Every fix canary-verified by reverting it individually. One existing assertion (the tenant-scope check on guidebook_sponsors) was a positional list that a new read would have silently weakened; restated as "every access is scoped by both id and org_id". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- .semgrep/baseline-counts.json | 4 +- app/actions/guidebook.ts | 123 +++++++++++++++- .../functions/guidebook-sponsor-activated.ts | 39 ++++- .../supabase-error-handling.test.ts | 5 +- unit/guidebook/guidebook-actions.test.ts | 139 +++++++++++++++++- .../guidebook-sponsor-activated.test.ts | 86 ++++++++++- 6 files changed, 381 insertions(+), 15 deletions(-) diff --git a/.semgrep/baseline-counts.json b/.semgrep/baseline-counts.json index a901ecde..dd21f6e1 100644 --- a/.semgrep/baseline-counts.json +++ b/.semgrep/baseline-counts.json @@ -99,8 +99,8 @@ ], "measured_at": "2026-08-07", "counts": { - "fieldstay-supabase-discarded-result": 80, - "fieldstay-supabase-read-without-error": 158, + "fieldstay-supabase-discarded-result": 79, + "fieldstay-supabase-read-without-error": 157, "fieldstay-supabase-read-without-error-fan-in": 0, "fieldstay-supabase-unbounded-select-in-list": 34, "fieldstay-supabase-unbounded-select-org-scoped": 98, diff --git a/app/actions/guidebook.ts b/app/actions/guidebook.ts index 41c97a2c..e517f278 100644 --- a/app/actions/guidebook.ts +++ b/app/actions/guidebook.ts @@ -1,6 +1,7 @@ 'use server' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' import { stripe } from '@/lib/stripe/client' import { requireOrgRole } from '@/lib/auth' import { inngest } from '@/lib/inngest/client' @@ -17,20 +18,78 @@ import { reportError } from '@/lib/observability/report-error' * invoked via /api/guidebook/sponsor-checkout rather than called * directly as a Server Action from a client component. */ +/** + * Sponsor states that still carry a LIVE Stripe subscription, so starting a + * fresh checkout would create a second one billing the same business. + * + * `payment_failed` is the non-obvious member. It is set from + * `invoice.payment_failed`, which does NOT end the subscription — Stripe keeps + * it in dunning, and guidebook-sponsor-payment-recovered flips the row + * straight back to 'active' when a retry succeeds. A sponsor who sees the + * failure notice and re-opens their media-kit link was therefore able to buy a + * SECOND subscription while the first was still being retried. + * + * 'cancelled' is deliberately absent: that subscription is gone, and buying + * again is the whole point of keeping the media kit link alive. + */ +const SPONSOR_STATUSES_WITH_LIVE_SUBSCRIPTION = ['active', 'payment_failed'] as const + +/** + * Returns the URL of an already-created Checkout Session if it is still + * payable, so a repeat click reuses it instead of minting another. + * + * Same helper the work-order invoice route has had all along + * (app/api/invoices/[invoiceId]/checkout/route.ts, "Store the session ID for + * potential reuse on duplicate clicks"). The sponsor path WROTE + * checkout_session_id for exactly this purpose and then never read it back + * from anywhere in the codebase — so every click, reload, or retry minted a + * new session, each payable for 24 hours. Two of them paid means two + * subscriptions, two checkout.session.completed webhooks with distinct event + * ids (so the dedup table does not collapse them), and an activation handler + * that overwrites stripe_subscription_id with whichever lands last — leaving + * the other subscription billing monthly with nothing in FieldStay able to + * cancel it. + */ +async function openSessionUrl(sessionId: string | null): Promise { + if (!sessionId) return null + try { + const existing = await stripe.checkout.sessions.retrieve(sessionId) + return existing.status === 'open' ? existing.url : null + } catch { + // Expired or not found — the caller mints a new one. + return null + } +} + export async function createSponsorCheckoutSession( mediaKitToken: string ): Promise<{ url: string } | { error: string }> { try { const supabase = createServiceClient({ publicSurface: 'guidebook-sponsor-media-kit' }) - const { data: sponsor } = await supabase + // maybeSingle + unwrap, not `{ data }` off .single(): discarding the error + // made an outage or an RLS regression indistinguishable from a bad token, + // so a sponsor holding a perfectly valid link was told it was invalid. + // Identical to the defect already fixed in optInGuestSms below — same + // file, one function over. + const sponsorRes = await supabase .from('guidebook_sponsors') - .select('id, org_id, business_name, slot_type, status') + .select('id, org_id, business_name, slot_type, status, checkout_session_id') .eq('media_kit_token', mediaKitToken) - .single() + .maybeSingle() + + const sponsor = unwrap(sponsorRes, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.sponsor', + }) + + if (!sponsor) return { error: 'Invalid media kit link.' } + + if ((SPONSOR_STATUSES_WITH_LIVE_SUBSCRIPTION as readonly string[]).includes(sponsor.status)) { + return { error: 'This sponsorship slot is already active.' } + } - if (!sponsor) return { error: 'Invalid media kit link.' } - if (sponsor.status === 'active') return { error: 'This sponsorship slot is already active.' } + const reusable = await openSessionUrl(sponsor.checkout_session_id) + if (reusable) return { url: reusable } const appUrl = process.env.NEXT_PUBLIC_APP_URL ?? 'https://app.fieldstay.app' @@ -57,7 +116,16 @@ export async function createSponsorCheckoutSession( if (!session.url) return { error: 'Stripe did not return a checkout URL.' } - await supabase + // Compare-and-swap on the value we read, not a blind overwrite. The reuse + // check above closes the repeat-click case; this closes the concurrent one + // — two requests that both saw no reusable session and both created one. + // Whichever swap matches the value it read owns the slot; the loser expires + // its own session and hands back the winner's, so only ONE payable session + // for this sponsor exists at a time. A plain `if (already active)` before + // the write is exactly the TOCTOU the audit checklist calls out: the + // precondition has to be in the WHERE clause. + const priorSessionId = sponsor.checkout_session_id + const claim = supabase .from('guidebook_sponsors') .update({ checkout_session_id: session.id, @@ -66,6 +134,49 @@ export async function createSponsorCheckoutSession( .eq('id', sponsor.id) .eq('org_id', sponsor.org_id) // explicit tenant guard + const claimRes = await ( + priorSessionId + ? claim.eq('checkout_session_id', priorSessionId) + : claim.is('checkout_session_id', null) + ) + .select('id') + .maybeSingle() + + const claimed = unwrap(claimRes, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.claim', + orgId: sponsor.org_id, + }) + + if (!claimed) { + // Lost the swap: a concurrent request stored its session after we read. + // Expire ours so it can never be paid, and return theirs. + await stripe.checkout.sessions.expire(session.id).catch((err: unknown) => { + // Non-fatal, but it must not be silent — an un-expired orphan session + // is precisely the double-subscription risk this block exists to close. + reportError(err, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.expire-orphan', + orgId: sponsor.org_id, + }) + }) + + const currentRes = await supabase + .from('guidebook_sponsors') + .select('checkout_session_id') + .eq('id', sponsor.id) + .eq('org_id', sponsor.org_id) + .maybeSingle() + + const winnerUrl = await openSessionUrl( + unwrap(currentRes, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.reread', + orgId: sponsor.org_id, + })?.checkout_session_id ?? null + ) + + if (winnerUrl) return { url: winnerUrl } + return { error: 'Unable to start checkout. Please try again.' } + } + // Unauthenticated flow (media kit page has no PM session) — no actorId await logAuditEvent({ orgId: sponsor.org_id, diff --git a/lib/inngest/functions/guidebook-sponsor-activated.ts b/lib/inngest/functions/guidebook-sponsor-activated.ts index 5d522e64..d78adee2 100644 --- a/lib/inngest/functions/guidebook-sponsor-activated.ts +++ b/lib/inngest/functions/guidebook-sponsor-activated.ts @@ -3,6 +3,7 @@ import { inngest } from '@/lib/inngest/client' import { createServiceClient } from '@/lib/supabase/server' import { getActiveSponsorCount } from '@/lib/guidebook/helpers' import { logAuditEvent } from '@/lib/audit' +import { reportError } from '@/lib/observability/report-error' export const guidebookSponsorActivated = inngest.createFunction( { id: 'guidebook-sponsor-activated', name: 'Guidebook: Sponsor Activated' }, @@ -12,6 +13,35 @@ export const guidebookSponsorActivated = inngest.createFunction( await step.run('activate-sponsor-row', async () => { const supabase = createServiceClient({ system: 'inngest:guidebook-sponsor-activated' }) + + // Read before the write purely to catch the anomaly below. Replacing one + // non-null subscription id with a DIFFERENT one means this sponsor now + // has two live Stripe subscriptions and FieldStay can only ever see the + // second — the first keeps billing the business monthly with nothing + // here able to cancel it. createSponsorCheckoutSession now reuses an + // open session and compare-and-swaps the new one, so our own flow should + // not produce this; a subscription created outside it still can, and it + // must not pass silently. + const existingRes = await supabase + .from('guidebook_sponsors') + .select('stripe_subscription_id') + .eq('id', sponsorId) + .eq('org_id', orgId) + .maybeSingle() + + const existing = unwrap(existingRes, { + site: 'inngest.guidebook-sponsor-activated.existing-subscription', orgId, + }) + + const priorSubscriptionId = existing?.stripe_subscription_id ?? null + if (priorSubscriptionId && priorSubscriptionId !== subscriptionId) { + reportError(new Error('Sponsor activated with a second Stripe subscription'), { + site: 'inngest.guidebook-sponsor-activated.duplicate-subscription', + orgId, + extra: { sponsor_id: sponsorId, prior_subscription_id: priorSubscriptionId, new_subscription_id: subscriptionId }, + }) + } + const { error } = await supabase .from('guidebook_sponsors') .update({ @@ -53,7 +83,12 @@ export const guidebookSponsorActivated = inngest.createFunction( if (!inTrial && activeSponsorCount < 3) return false - await supabase + // Bound and thrown, matching the identical upsert in + // guidebook-sponsor-payment-recovered. Discarded, a failed unlock left + // the guidebook locked while this step returned `wasUnlocked: true` and + // the audit row recorded an unlock that never happened — the sponsor + // paid and the guidebook stayed dark. + const { error } = await supabase .from('guidebook_configurations') .upsert( { @@ -65,6 +100,8 @@ export const guidebookSponsorActivated = inngest.createFunction( { onConflict: 'org_id' } ) + if (error) throw new Error(`Failed to unlock guidebook: ${error.message}`) + return true }) diff --git a/unit/guardrails/supabase-error-handling.test.ts b/unit/guardrails/supabase-error-handling.test.ts index 0c17daaf..dbe8f917 100644 --- a/unit/guardrails/supabase-error-handling.test.ts +++ b/unit/guardrails/supabase-error-handling.test.ts @@ -109,7 +109,10 @@ const BASELINE: Record = { // 3 -> 2: optInGuestSms's booking lookup now binds and reports its error. // Discarding it made a transient failure indistinguishable from a bad token, // so a guest with a valid link was told the link was invalid. - 'app/actions/guidebook.ts': 2, + // 2 -> 1: createSponsorCheckoutSession's media-kit-token lookup had the SAME + // defect one function over — a sponsor holding a valid link was told it was + // invalid whenever the query itself failed. + 'app/actions/guidebook.ts': 1, 'app/api/repuguard/generate/route.ts': 3, 'app/api/vendor-connect/[token]/onboard/route.ts': 2, diff --git a/unit/guidebook/guidebook-actions.test.ts b/unit/guidebook/guidebook-actions.test.ts index cd9dbff3..4dbd9e63 100644 --- a/unit/guidebook/guidebook-actions.test.ts +++ b/unit/guidebook/guidebook-actions.test.ts @@ -7,7 +7,11 @@ vi.mock('@/lib/supabase/server', () => ({ createServiceClient: vi.fn(), })) vi.mock('@/lib/stripe/client', () => ({ - stripe: { checkout: { sessions: { create: vi.fn() } } }, + // retrieve/expire back the duplicate-session defence in + // createSponsorCheckoutSession: reuse an already-open session rather than + // minting a second payable one, and expire our own if we lose the + // compare-and-swap to a concurrent request. + stripe: { checkout: { sessions: { create: vi.fn(), retrieve: vi.fn(), expire: vi.fn() } } }, })) vi.mock('@/lib/inngest/client', () => ({ inngest: { send: vi.fn() } })) vi.mock('@/lib/audit', () => ({ logAuditEvent: vi.fn() })) @@ -41,7 +45,11 @@ function makeSupabase(queue: Record) { // `.not(` is the global-STOP consent check // (.not('opted_out_at', 'is', null)); without it the chain call throws and // every opt-in test collapses into the generic catch. - for (const m of ['select', 'insert', 'update', 'upsert', 'eq', 'not', 'order', 'limit']) { + // `.is(` is the compare-and-swap precondition on a sponsor row that has no + // stored checkout session yet (.is('checkout_session_id', null)); without + // it the chain call throws and the whole action collapses into its generic + // catch, which reads as a Stripe failure. + for (const m of ['select', 'insert', 'update', 'upsert', 'eq', 'is', 'not', 'order', 'limit']) { chain[m] = vi.fn((...args: unknown[]) => { calls.push({ table, method: m, args }) return chain @@ -70,7 +78,8 @@ describe('actions/guidebook', () => { it('creates a Stripe checkout session for a valid, inactive sponsor slot', async () => { const supabase = makeSupabase({ guidebook_sponsors: [ - { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending' } }, + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: null } }, + { data: { id: 'sponsor_1' } }, // compare-and-swap of the session id wins ], }) vi.mocked(createServiceClient).mockReturnValue(supabase as never) @@ -126,6 +135,130 @@ describe('actions/guidebook', () => { expect(result).toEqual({ error: 'Unable to start checkout. Please try again.' }) }) + + // ── Duplicate-subscription defences ────────────────────────────────── + // Two payable sessions for one sponsor means two subscriptions, two + // checkout.session.completed webhooks with distinct event ids (so the + // Stripe dedup table does not collapse them), and an activation handler + // that overwrites stripe_subscription_id with whichever lands last — + // leaving the other one billing monthly with nothing able to cancel it. + + it('reuses an already-open checkout session instead of minting a second payable one', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: 'cs_prior' } }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.retrieve).mockResolvedValue({ + id: 'cs_prior', status: 'open', url: 'https://checkout.stripe.com/cs_prior', + } as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + // checkout_session_id was written for exactly this and then never read + // back anywhere, so every click, reload, and retry minted a new session + // — each payable for 24 hours. + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_prior' }) + expect(stripe.checkout.sessions.create).not.toHaveBeenCalled() + }) + + it('mints a new session when the stored one is no longer open', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: 'cs_expired' } }, + { data: { id: 'sponsor_1' } }, // compare-and-swap wins + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.retrieve).mockResolvedValue({ + id: 'cs_expired', status: 'expired', url: null, + } as never) + vi.mocked(stripe.checkout.sessions.create).mockResolvedValue({ + id: 'cs_2', url: 'https://checkout.stripe.com/cs_2', + } as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_2' }) + }) + + it('expires its own session and returns the winner\'s when it loses the compare-and-swap', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: null } }, + { data: null }, // CAS matched 0 rows — a concurrent request got there first + { data: { checkout_session_id: 'cs_winner' } }, // re-read finds the winner's session + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.create).mockResolvedValue({ + id: 'cs_loser', url: 'https://checkout.stripe.com/cs_loser', + } as never) + vi.mocked(stripe.checkout.sessions.retrieve).mockResolvedValue({ + id: 'cs_winner', status: 'open', url: 'https://checkout.stripe.com/cs_winner', + } as never) + vi.mocked(stripe.checkout.sessions.expire).mockResolvedValue({} as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + // A plain `if (already active)` before the write is the exact TOCTOU the + // audit checklist calls out — the precondition has to be in the WHERE + // clause, and the loser has to clean up after itself. + expect(stripe.checkout.sessions.expire).toHaveBeenCalledWith('cs_loser') + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_winner' }) + }) + + it('refuses a sponsor in payment_failed — its subscription is still live in dunning', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'payment_failed', checkout_session_id: null } }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + // payment_failed comes from invoice.payment_failed, which does NOT end + // the subscription — Stripe keeps retrying, and + // guidebook-sponsor-payment-recovered flips the row back to 'active'. + // Only 'active' was blocked, so a sponsor who saw the failure notice + // could buy a SECOND subscription while the first was still in dunning. + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ error: 'This sponsorship slot is already active.' }) + expect(stripe.checkout.sessions.create).not.toHaveBeenCalled() + }) + + it('allows a cancelled sponsor to buy again — that subscription is genuinely gone', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'cancelled', checkout_session_id: null } }, + { data: { id: 'sponsor_1' } }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.create).mockResolvedValue({ + id: 'cs_3', url: 'https://checkout.stripe.com/cs_3', + } as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_3' }) + }) + + it('does not tell a sponsor their valid link is invalid when the lookup itself fails', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [{ data: null, error: { message: 'connection reset', code: '08006' } }], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + // Discarding the read error collapsed "no such token" and "the query + // failed" into the same null — identical to the defect already fixed in + // optInGuestSms, one function over in the same file. + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ error: 'Unable to start checkout. Please try again.' }) + expect(stripe.checkout.sessions.create).not.toHaveBeenCalled() + }) }) describe('upsertSponsor', () => { diff --git a/unit/inngest/guidebook-sponsor-activated.test.ts b/unit/inngest/guidebook-sponsor-activated.test.ts index 120cf84d..45a4ebd9 100644 --- a/unit/inngest/guidebook-sponsor-activated.test.ts +++ b/unit/inngest/guidebook-sponsor-activated.test.ts @@ -9,11 +9,15 @@ vi.mock('@/lib/guidebook/helpers', () => ({ vi.mock('@/lib/audit', () => ({ logAuditEvent: vi.fn(), })) +vi.mock('@/lib/observability/report-error', () => ({ + reportError: vi.fn(), +})) import { guidebookSponsorActivated } from '@/lib/inngest/functions/guidebook-sponsor-activated' import { createServiceClient } from '@/lib/supabase/server' import { getActiveSponsorCount } from '@/lib/guidebook/helpers' import { logAuditEvent } from '@/lib/audit' +import { reportError } from '@/lib/observability/report-error' import { invokeHandler } from './test-helpers' // Queue-based `.from(table)` mock — same convention as checklist-broadcast @@ -92,8 +96,18 @@ describe('guidebookSponsorActivated', () => { stripe_subscription_id: 'sub_1', stripe_customer_id: 'cus_1', }) - const sponsorEqCalls = supabase.calls.filter((c) => c.table === 'guidebook_sponsors' && c.method === 'eq') - expect(sponsorEqCalls.map((c) => c.args)).toEqual([['id', 'sponsor_1'], ['org_id', 'org_1']]) + // Stated as "every guidebook_sponsors access is scoped by BOTH id and + // org_id" rather than as a positional list, so adding a read to this step + // cannot quietly weaken it into passing on a subset. + const sponsorEqArgs = supabase.calls + .filter((c) => c.table === 'guidebook_sponsors' && c.method === 'eq') + .map((c) => JSON.stringify(c.args)) + expect(sponsorEqArgs.filter((a) => a === JSON.stringify(['id', 'sponsor_1'])).length) + .toBe(sponsorEqArgs.filter((a) => a === JSON.stringify(['org_id', 'org_1'])).length) + expect(sponsorEqArgs.length).toBeGreaterThanOrEqual(2) + expect(new Set(sponsorEqArgs)).toEqual( + new Set([JSON.stringify(['id', 'sponsor_1']), JSON.stringify(['org_id', 'org_1'])]), + ) const configUpsert = supabase.calls.find((c) => c.table === 'guidebook_configurations' && c.method === 'upsert') expect(configUpsert?.args[0]).toMatchObject({ org_id: 'org_1', is_active: true, grace_period_ends_at: null }) @@ -171,4 +185,72 @@ describe('guidebookSponsorActivated', () => { expect(supabase.calls.filter((c) => c.table === 'guidebook_sponsors' && c.method === 'update')).toHaveLength(2) expect(logAuditEvent).toHaveBeenCalledTimes(2) }) + + it('throws when the unlock upsert fails, instead of reporting an unlock that never happened', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [{ data: null, error: null }], + guidebook_configurations: [ + { data: { trial_ends_at: null }, error: null }, + { data: null, error: { message: 'permission denied for table guidebook_configurations' } }, + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getActiveSponsorCount as ReturnType).mockResolvedValue(3) + + // Discarded, a failed unlock left the guidebook locked while the step + // returned wasUnlocked: true and the audit row recorded an unlock that + // never happened — the sponsor paid and the guidebook stayed dark, with + // the record saying otherwise. Matches the identical upsert in + // guidebook-sponsor-payment-recovered, which always threw. + await expect( + invokeHandler(guidebookSponsorActivated, { event: checkoutEvent(), step: makeStep() }) + ).rejects.toThrow(/Failed to unlock guidebook/) + + expect(logAuditEvent).not.toHaveBeenCalled() + }) + + it('reports — rather than silently overwriting — when a second subscription id replaces a live one', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { stripe_subscription_id: 'sub_previous' }, error: null }, // already has a live subscription + { data: null, error: null }, // the activation update + ], + guidebook_configurations: [{ data: { trial_ends_at: null }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getActiveSponsorCount as ReturnType).mockResolvedValue(1) + + await invokeHandler(guidebookSponsorActivated, { event: checkoutEvent(), step: makeStep() }) + + // Overwriting stripe_subscription_id orphans the previous subscription: + // it keeps billing the business monthly and nothing in FieldStay can + // reach it any more. The write still proceeds (the new subscription is + // the real one now) but the anomaly must not pass unnoticed. + expect(reportError).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Sponsor activated with a second Stripe subscription' }), + expect.objectContaining({ + site: 'inngest.guidebook-sponsor-activated.duplicate-subscription', + extra: expect.objectContaining({ + prior_subscription_id: 'sub_previous', + new_subscription_id: 'sub_1', + }), + }), + ) + }) + + it('does not report when a replay writes back the SAME subscription id', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { stripe_subscription_id: 'sub_1' }, error: null }, // same as the event's + { data: null, error: null }, + ], + guidebook_configurations: [{ data: { trial_ends_at: null }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getActiveSponsorCount as ReturnType).mockResolvedValue(1) + + await invokeHandler(guidebookSponsorActivated, { event: checkoutEvent(), step: makeStep() }) + + expect(reportError).not.toHaveBeenCalled() + }) }) From 187524f9663e56acd13f5560d9cdb6595dda3c12 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 16:10:25 +0000 Subject: [PATCH 05/11] fix(guidebook): validate uuid input at the redeem boundary; share UUID_RE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/guidebook/redeem` is public and unauthenticated, and both ids it takes land in `uuid` columns — `guidebook_sponsors.id` and `bookings.guidebook_token`. Postgres does not coerce: a non-UUID string is error 22P02, not an empty result. The route only checked `typeof === 'string'`. - A malformed `sponsorId` reached `.eq('id', …)`, 22P02'd, threw out of unwrap(), and the catch turned it into `{ok:true}` PLUS a reportError. On an unauthenticated endpoint that is a free way for anyone to burn the Sentry quota and bury this route's genuine database failures in the noise — and the limiter bounds it only while Redis is up, since this one deliberately fails open. A malformed id is a bad request, so it now 400s before touching the DB. - A malformed `bookingToken` was worse than unattributed. The 22P02 escaped the try block entirely and hit the outer catch, so the redemption INSERT never ran — the redemption was discarded, not merely anonymous, while the guest still saw `{ok:true}`. The route's own stated fallback three lines down is an anonymous redemption; an unusable token now skips the booking lookup and takes exactly that path. The identical UUID regex had been open-coded in four files (two crew routes, crew work-order complete, lib/storage/object-path.ts) and was simply missing from the fifth place that needed it, so it moves to lib/validation/uuid.ts and those four now import it. Five copies is how the sixth gets forgotten. Test-fixture note, and the reason the second fix nearly shipped unverified: this suite used ids like 'sponsor_1' and 'tok_abc' that could never exist in production — against the live schema they are 22P02, not a miss. So the mock was accepting input the database rejects, and the first version of the malformed-token test PASSED with the fix reverted. The double now answers 22P02 for a non-UUID filtered against a uuid column, which is what Postgres does, and both canaries bite. guidebook_offer_redemptions has zero rows in production and nothing reads it yet, so no collected data was affected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- app/api/crew/inventory-count/route.ts | 3 +- app/api/crew/messages/route.ts | 3 +- .../crew/work-orders/[id]/complete/route.ts | 3 +- app/api/guidebook/redeem/route.ts | 22 ++++- lib/storage/object-path.ts | 4 +- lib/validation/uuid.ts | 41 ++++++++++ unit/route-handlers/guidebook-redeem.test.ts | 82 ++++++++++++++++--- 7 files changed, 141 insertions(+), 17 deletions(-) create mode 100644 lib/validation/uuid.ts diff --git a/app/api/crew/inventory-count/route.ts b/app/api/crew/inventory-count/route.ts index e033ec7b..4cc5ade4 100644 --- a/app/api/crew/inventory-count/route.ts +++ b/app/api/crew/inventory-count/route.ts @@ -4,6 +4,7 @@ import { inngest } from '@/lib/inngest/client' import { logAuditEvents } from '@/lib/audit' import { reportQueryError, unwrapList } from '@/lib/supabase/unwrap' import { fetchAllRows } from '@/lib/inngest/paginate' +import { UUID_RE } from '@/lib/validation/uuid' /** * POST /api/crew/inventory-count @@ -37,7 +38,7 @@ interface CountSubmission { itemNotes?: Record } -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + /** Double-tap window for clients that submit without a count id. */ const DEDUP_WINDOW_MS = 5 * 60 * 1000 diff --git a/app/api/crew/messages/route.ts b/app/api/crew/messages/route.ts index aa0f9fd0..9fc990a8 100644 --- a/app/api/crew/messages/route.ts +++ b/app/api/crew/messages/route.ts @@ -4,6 +4,7 @@ import { requireCrewMember } from '@/lib/crew-auth' import { createServiceClient } from '@/lib/supabase/server' import { getPmMembers } from '@/lib/inngest/helpers' import { reportError } from '@/lib/observability/report-error' +import { UUID_RE } from '@/lib/validation/uuid' /** * POST /api/crew/messages @@ -110,7 +111,7 @@ async function notifyPmSlack( * (/api/crew/inventory-count) already validates its own, and this one is the * copy that did not. */ -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + /** * `content` is unbounded `text` in the database and has no maxLength on the diff --git a/app/api/crew/work-orders/[id]/complete/route.ts b/app/api/crew/work-orders/[id]/complete/route.ts index 8a125973..2a7afa63 100644 --- a/app/api/crew/work-orders/[id]/complete/route.ts +++ b/app/api/crew/work-orders/[id]/complete/route.ts @@ -11,6 +11,7 @@ import { type CompletedWorkOrderRow, } from '@/app/(dashboard)/maintenance/complete-work-order-helpers' import type { WoStatus } from '@/types/database' +import { UUID_RE } from '@/lib/validation/uuid' /** * POST /api/crew/work-orders/[id]/complete @@ -33,7 +34,7 @@ import type { WoStatus } from '@/types/database' * Not reachable from our own client (the PWA builds the URL from cached * crew_work_orders ids), which is why it is asserted rather than assumed. */ -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + /** Free text typed on a phone into an unbounded `completion_notes` column. */ const MAX_NOTES_LENGTH = 2000 diff --git a/app/api/guidebook/redeem/route.ts b/app/api/guidebook/redeem/route.ts index fb903dce..0374e4e8 100644 --- a/app/api/guidebook/redeem/route.ts +++ b/app/api/guidebook/redeem/route.ts @@ -4,6 +4,7 @@ import { guidebookRedeemLimiter, checkLimit } from '@/lib/rate-limit' import { extractClientIp } from '@/lib/integrations/webhook-verification' import { reportError } from '@/lib/observability/report-error' import { unwrap } from '@/lib/supabase/unwrap' +import { isUuid } from '@/lib/validation/uuid' /** * POST /api/guidebook/redeem @@ -27,7 +28,16 @@ export async function POST(req: NextRequest): Promise { } const body = await req.json().catch(() => null) as { sponsorId?: string; bookingToken?: string | null } | null - if (!body?.sponsorId || typeof body.sponsorId !== 'string') { + + // Shape-checked before it reaches a `uuid` column, not just type-checked. + // `guidebook_sponsors.id` is a uuid, so a non-UUID string is Postgres 22P02 + // — which unwrap() below turns into a throw, and the catch turns into + // `{ok:true}` PLUS a Sentry report. On a public, unauthenticated endpoint + // that is a free way for anyone to burn the Sentry quota and bury this + // route's real database failures in noise; the limiter bounds it only while + // Redis is up, and this one deliberately fails OPEN. A malformed id is a bad + // request, so say so. + if (!isUuid(body?.sponsorId)) { return NextResponse.json({ error: 'sponsorId is required' }, { status: 400 }) } @@ -47,7 +57,15 @@ export async function POST(req: NextRequest): Promise { } let bookingId: string | null = null - if (body.bookingToken && typeof body.bookingToken === 'string') { + // isUuid, not just a truthy string check — and note this one SKIPS rather + // than 400s. `bookings.guidebook_token` is also a uuid, so a malformed + // token threw 22P02 out of unwrap(), escaped the try entirely, and landed + // in the outer catch: the guest got `{ok:true}` and the redemption insert + // below never ran at all. The stated intent two lines down is to fall back + // to an ANONYMOUS redemption when the booking can't be attributed, so + // that is what an unusable token should do — degrade attribution, not + // discard the redemption. + if (isUuid(body.bookingToken)) { const bookingRes = await supabase .from('bookings') .select('id, org_id') diff --git a/lib/storage/object-path.ts b/lib/storage/object-path.ts index 171ccf2c..d43795f3 100644 --- a/lib/storage/object-path.ts +++ b/lib/storage/object-path.ts @@ -20,12 +20,12 @@ // components, Server Actions, Route Handlers, and Inngest steps alike. No // `server-only`, no Supabase import. +import { UUID_RE } from '@/lib/validation/uuid' + export const ORG_SCOPED_PHOTO_BUCKETS = ['work-order-photos', 'turnover-photos'] as const export type OrgScopedPhotoBucket = (typeof ORG_SCOPED_PHOTO_BUCKETS)[number] -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i - /** * Builds an object key for an org-scoped photo bucket. * diff --git a/lib/validation/uuid.ts b/lib/validation/uuid.ts new file mode 100644 index 00000000..50905c82 --- /dev/null +++ b/lib/validation/uuid.ts @@ -0,0 +1,41 @@ +// lib/validation/uuid.ts +// +// The UUID shape check, in one place. +// +// Every id this app puts into a `.eq()` is a Postgres `uuid` column, and +// Postgres does not coerce — a non-UUID string is error 22P02 ("invalid input +// syntax for type uuid"), not an empty result. What that error becomes depends +// entirely on where it lands: +// +// - through unwrap(), it throws — which in a public route means a Sentry +// report per malformed request, so anyone can burn the quota and bury the +// real DB failures on that route under noise; +// - destructured as `{ data }`, it silently reads as "no such row". +// +// Neither is what the caller meant. A malformed id is a BAD REQUEST and +// belongs to input validation at the boundary — the item CLAUDE.md's manual +// audit checklist lists as having no mechanical guardrail. +// +// This regex was independently open-coded in four files before this module +// existed (two crew routes, the crew work-order complete route, and +// lib/storage/object-path.ts) and was simply missing from the fifth place that +// needed it. Five copies is how the sixth one gets forgotten too. +// +// Deliberately NOT `import 'server-only'`: a pure predicate over a string, +// usable from a Client Component that wants to reject input before a round +// trip. Same rationale as lib/storage/object-path.ts and lib/supabase/unwrap.ts. + +/** + * Matches the canonical 8-4-4-4-12 hex form, case-insensitively. + * + * Intentionally not version- or variant-aware: the job is to keep malformed + * input out of a `uuid` column, and Postgres accepts any 8-4-4-4-12 hex string + * regardless of the version nibble. A stricter pattern would reject ids + * Postgres itself considers valid. + */ +export const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + +/** True when `value` is a string Postgres will accept for a `uuid` column. */ +export function isUuid(value: unknown): value is string { + return typeof value === 'string' && UUID_RE.test(value) +} diff --git a/unit/route-handlers/guidebook-redeem.test.ts b/unit/route-handlers/guidebook-redeem.test.ts index 0081c0d9..a94c82cb 100644 --- a/unit/route-handlers/guidebook-redeem.test.ts +++ b/unit/route-handlers/guidebook-redeem.test.ts @@ -20,9 +20,20 @@ import { POST } from '@/app/api/guidebook/redeem/route' import { createServiceClient } from '@/lib/supabase/server' import { guidebookRedeemLimiter } from '@/lib/rate-limit' -const SPONSOR_ID = 'sponsor_1' -const ORG_ID = 'org_1' -const OTHER_ORG = 'org_2' +// Real UUIDs, because every one of these ids is a Postgres `uuid` column and +// the route now shape-checks before querying. The previous fixtures +// ('sponsor_1', 'tok_abc') could not have existed in production — against the +// live schema they are error 22P02, not a miss, so the suite was green on +// inputs the database would have rejected outright. +const SPONSOR_ID = '11111111-1111-4111-8111-111111111111' +const ORG_ID = '22222222-2222-4222-8222-222222222222' +const OTHER_ORG = '33333333-3333-4333-8333-333333333333' +const BOOKING_ID = '44444444-4444-4444-8444-444444444444' +const BOOKING_TOK = '55555555-5555-4555-8555-555555555555' + +/** Columns the live schema declares as `uuid` — see the mock's 22P02 branch. */ +const UUID_COLUMNS = new Set(['id', 'guidebook_token']) +const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i function makeServiceClient(opts: { sponsorResult?: { data: unknown; error?: unknown } @@ -34,9 +45,28 @@ function makeServiceClient(opts: { const from = vi.fn((table: string) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any const chain: any = {} + const eqArgs: [string, unknown][] = [] chain.select = vi.fn(() => chain) - chain.eq = vi.fn(() => chain) + chain.eq = vi.fn((col: string, val: unknown) => { eqArgs.push([col, val]); return chain }) chain.maybeSingle = vi.fn(() => { + // The double answers 22P02 for a non-UUID filtered against a `uuid` + // column, because that is what Postgres does — it does NOT return zero + // rows. Without this the mock silently accepts ids the real database + // rejects, and any test about malformed-input handling passes whether + // the handling exists or not. + const badUuid = eqArgs.find( + ([col, val]) => UUID_COLUMNS.has(col) && !UUID_RE.test(String(val)), + ) + if (badUuid) { + return Promise.resolve({ + data: null, + error: { + code: '22P02', + message: `invalid input syntax for type uuid: "${String(badUuid[1])}"`, + }, + }) + } + if (table === 'guidebook_sponsors') { return Promise.resolve( opts.sponsorResult ?? { data: { id: SPONSOR_ID, org_id: ORG_ID, status: 'active' }, error: null }, @@ -82,6 +112,38 @@ describe('POST /api/guidebook/redeem', () => { expect(createServiceClient).not.toHaveBeenCalled() }) + it('returns 400 for a malformed sponsorId rather than letting Postgres 22P02 become a Sentry report', async () => { + const res = await POST(postRequest({ sponsorId: 'sponsor_1' })) + + // A well-formed string that is not a UUID used to sail past the + // `typeof === 'string'` check straight into `.eq('id', …)` on a uuid + // column. Postgres answers 22P02, unwrap() throws, and the catch returns + // {ok:true} PLUS a reportError — so on a public unauthenticated endpoint + // anyone could burn the Sentry quota, and this route's genuine DB failures + // would be buried in that noise. + expect(res.status).toBe(400) + expect(createServiceClient).not.toHaveBeenCalled() + }) + + it('still logs the redemption — anonymously — when the bookingToken is malformed', async () => { + const service = makeServiceClient() + vi.mocked(createServiceClient).mockReturnValue(service as never) + + const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: 'tok_abc' })) + + // bookings.guidebook_token is a uuid too. A malformed one threw 22P02 out + // of unwrap(), escaped the try block entirely, and hit the outer catch — + // so the insert never ran and the redemption was DISCARDED, not just left + // unattributed, while the guest still saw {ok:true}. The route's own + // stated fallback is an anonymous redemption; this is it. + expect(res.status).toBe(200) + expect(service.insertMock).toHaveBeenCalledWith({ + org_id: ORG_ID, + sponsor_id: SPONSOR_ID, + booking_id: null, + }) + }) + it('returns 429 when the per-IP rate limit is exceeded, without querying the database', async () => { vi.mocked(guidebookRedeemLimiter.limit).mockResolvedValue({ success: false } as never) @@ -95,7 +157,7 @@ describe('POST /api/guidebook/redeem', () => { const service = makeServiceClient({ sponsorResult: { data: null, error: null } }) vi.mocked(createServiceClient).mockReturnValue(service as never) - const res = await POST(postRequest({ sponsorId: 'nonexistent' })) + const res = await POST(postRequest({ sponsorId: '99999999-9999-4999-8999-999999999999' })) const json = await res.json() expect(res.status).toBe(200) @@ -133,27 +195,27 @@ describe('POST /api/guidebook/redeem', () => { it('attaches the booking_id when the booking token resolves to a booking in the sponsor\'s own org', async () => { const service = makeServiceClient({ - bookingResult: { data: { id: 'booking_1', org_id: ORG_ID }, error: null }, + bookingResult: { data: { id: BOOKING_ID, org_id: ORG_ID }, error: null }, }) vi.mocked(createServiceClient).mockReturnValue(service as never) - const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: 'tok_abc' })) + const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) expect(res.status).toBe(200) expect(service.insertMock).toHaveBeenCalledWith({ org_id: ORG_ID, sponsor_id: SPONSOR_ID, - booking_id: 'booking_1', + booking_id: BOOKING_ID, }) }) it('logs anonymously (booking_id: null) when the booking token belongs to a different org — tenant isolation', async () => { const service = makeServiceClient({ - bookingResult: { data: { id: 'booking_1', org_id: OTHER_ORG }, error: null }, + bookingResult: { data: { id: BOOKING_ID, org_id: OTHER_ORG }, error: null }, }) vi.mocked(createServiceClient).mockReturnValue(service as never) - const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: 'tok_cross_org' })) + const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) expect(res.status).toBe(200) expect(service.insertMock).toHaveBeenCalledWith({ From e196298023a613f84555f0724dec731f28ea6bc1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 16:22:47 +0000 Subject: [PATCH 06/11] feat(guidebook): dedupe sponsor redemptions to one per booking per day MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirmed the interaction: tapping a sponsor offer opens the redemption pass — a full-screen "Guest Perk · Verified Live" card with a live clock, captioned "Show this screen to staff — clock proves it's live". It is a coupon presented at the counter, and its row count is the number a paying sponsor will judge their slot by. Opening that pass more than once is the normal case, not an edge case: a guest looks at the offer from the couch, closes it, walks to the business, and opens it again to show staff. Every reopen was its own row, so COUNT(*) overstated real redemptions by however many times the guest looked at their own coupon. - New partial unique index on (sponsor_id, booking_id, UTC date of opened_at), applied to production AND the E2E project with matching ledger rows. - The route treats 23505 as the success path — already counted today. Caught rather than expressed as upsert({onConflict}) because the index is an EXPRESSION index and PostgREST's on_conflict only takes plain column names. Per DAY, not per stay: a daily perk is legitimately redeemable on each day of a booking, so collapsing a whole stay would under-count. The UTC day boundary splits a single evening's repeat opens only when they straddle UTC midnight (6-8pm US local) — that errs toward one extra, never toward discarding a genuinely distinct day, which is the safe direction here. Anonymous redemptions (booking_id NULL, from the property-level /g/[slug] guidebook, which has no booking token) are deliberately outside the predicate: with no guest identity there is nothing to dedupe on, and collapsing them by (sponsor, day) would merge different guests into one. Guardrail: the constraint and the handler branch live in different files with nothing tying them together, and both drift directions are silent — drop the index and the 23505 branch becomes dead code while counts quietly inflate; drop the branch and every reopen becomes a reported error, moving the noise from the sponsor's number into Sentry. unit/guardrails/redemption-dedup-pairing asserts they stay together. Both directions canary-verified. Verified zero conflicting rows in production before applying. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- app/api/guidebook/redeem/route.ts | 25 +++++++ ...uidebook_offer_redemptions_daily_dedup.sql | 44 +++++++++++ .../redemption-dedup-pairing.test.ts | 74 +++++++++++++++++++ unit/route-handlers/guidebook-redeem.test.ts | 43 +++++++++++ 4 files changed, 186 insertions(+) create mode 100644 supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql create mode 100644 unit/guardrails/redemption-dedup-pairing.test.ts diff --git a/app/api/guidebook/redeem/route.ts b/app/api/guidebook/redeem/route.ts index 0374e4e8..1dd3e054 100644 --- a/app/api/guidebook/redeem/route.ts +++ b/app/api/guidebook/redeem/route.ts @@ -83,6 +83,31 @@ export async function POST(req: NextRequest): Promise { booking_id: bookingId, }) + // 23505 against uniq_guidebook_offer_redemptions_sponsor_booking_day + // (migration 20260807150000) is the SUCCESS path, not a failure: this + // sponsor is already counted for this booking today. + // + // The row is written when the guest opens the redemption pass — the coupon + // they show at the counter — and opening it more than once is the NORMAL + // case: look at the offer from the couch, close it, walk over, open it + // again for staff. Counting each reopen inflated the exact number a paying + // sponsor judges their slot by. + // + // Per day, not per stay: a daily perk is legitimately redeemable on each + // day of a booking, so collapsing the whole stay would under-count. + // + // Caught here rather than expressed as upsert({onConflict}): the index is + // an EXPRESSION index (on the UTC date of opened_at), and PostgREST's + // on_conflict only takes plain column names. + // + // Anonymous redemptions (bookingId null, from the property-level + // /g/[slug] guidebook) fall outside the partial index by design — with no + // guest identity there is nothing to dedupe on, and collapsing them by + // (sponsor, day) would merge different guests into one. + if (error?.code === '23505') { + return NextResponse.json({ ok: true }) + } + if (error) { // Redemption logging is best-effort — never fail the guest UX over it — // but it must not be SILENT either: without the code/message here, a diff --git a/supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql b/supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql new file mode 100644 index 00000000..341f6fd9 --- /dev/null +++ b/supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql @@ -0,0 +1,44 @@ +-- guidebook_offer_redemptions: one redemption per sponsor, per booking, per day. +-- +-- The row is written when a guest opens the redemption pass — the full-screen +-- "Guest Perk · Verified Live" card whose own hint reads "Show this screen to +-- staff — clock proves it's live". It is a coupon presented at the counter, not +-- a page-view, and it is the number a paying sponsor will judge their slot by. +-- +-- Opening that pass more than once is the normal case, not an edge case: a +-- guest looks at the offer from the couch, closes it, walks to the business, +-- and opens it again to show staff. Every reopen was a separate row, so raw +-- COUNT(*) overstated real redemptions by however many times the guest looked +-- at their own coupon. +-- +-- Nothing reads this table yet. That is exactly why the constraint goes in now: +-- whoever builds the sponsor report later will reach for COUNT(*), and the +-- honest number has to be the one the table can produce. +-- +-- Day, not stay: a "free coffee" perk is legitimately redeemable on each day of +-- a booking, so collapsing a whole stay to one row would UNDER-count. Within a +-- day, one redemption per sponsor. +-- +-- The day boundary is UTC because the row carries no timezone of its own. For a +-- US property this splits a single evening's repeat opens across two dates only +-- when they straddle UTC midnight (6-8pm local). That errs toward counting one +-- extra, never toward missing a genuinely distinct day — the safe direction for +-- a constraint whose failure mode would otherwise be silently discarding a real +-- redemption. +-- +-- Anonymous redemptions (booking_id IS NULL, from the property-level /g/[slug] +-- guidebook, which has no booking token) are deliberately NOT covered: with no +-- guest identity there is nothing to dedupe on, and collapsing them by +-- (sponsor, day) would merge DIFFERENT guests into one. They stay uncapped and +-- unattributed, which is what they are. + +CREATE UNIQUE INDEX IF NOT EXISTS uniq_guidebook_offer_redemptions_sponsor_booking_day + ON guidebook_offer_redemptions ( + sponsor_id, + booking_id, + ((opened_at AT TIME ZONE 'UTC')::date) + ) + WHERE booking_id IS NOT NULL; + +COMMENT ON INDEX uniq_guidebook_offer_redemptions_sponsor_booking_day IS + 'One redemption per sponsor per booking per UTC day. Repeat opens of the same pass are the same redemption; app/api/guidebook/redeem relies on this for ignoreDuplicates.'; diff --git a/unit/guardrails/redemption-dedup-pairing.test.ts b/unit/guardrails/redemption-dedup-pairing.test.ts new file mode 100644 index 00000000..cfb3a081 --- /dev/null +++ b/unit/guardrails/redemption-dedup-pairing.test.ts @@ -0,0 +1,74 @@ +// unit/guardrails/redemption-dedup-pairing.test.ts +// +// guidebook_offer_redemptions has a dedup constraint and a handler branch that +// depends on it, and they live in different files with nothing tying them +// together. +// +// The row is written when a guest opens the redemption pass — the coupon they +// show at the counter — and reopening it is the normal case, so raw COUNT(*) +// overstates real redemptions by however many times the guest looked at their +// own coupon. That count is what a paying sponsor judges their slot by. +// +// Two ways the pairing silently breaks: +// +// - the unique index goes away (migration reverted, index renamed, table +// rebuilt) and the route's 23505 branch becomes dead code. Nothing errors; +// the counts just quietly inflate again. +// - the route stops handling 23505 and every reopen becomes a reported +// error instead — the noise moves from the sponsor's number into Sentry, +// which is not an improvement. +// +// Neither shows up as a test failure anywhere else: no unit test exercises the +// real database, and the sponsor-facing report that would surface a wrong +// number does not exist yet. This asserts the two halves stay together. + +import { describe, it, expect } from 'vitest' +import { readFileSync, readdirSync } from 'node:fs' +import { join } from 'node:path' + +const ROOT = join(__dirname, '..', '..') +const ROUTE = 'app/api/guidebook/redeem/route.ts' +const MIGRATIONS_DIR = join(ROOT, 'supabase', 'migrations') +const INDEX_NAME = 'uniq_guidebook_offer_redemptions_sponsor_booking_day' + +function migrationSources(): string { + return readdirSync(MIGRATIONS_DIR) + .filter((f) => f.endsWith('.sql')) + .map((f) => readFileSync(join(MIGRATIONS_DIR, f), 'utf8')) + .join('\n') +} + +describe('guidebook redemption dedup — constraint and handler stay paired', () => { + it('a migration creates the unique index the handler relies on', () => { + const sql = migrationSources() + const createsIt = new RegExp( + `CREATE\\s+UNIQUE\\s+INDEX(\\s+IF\\s+NOT\\s+EXISTS)?\\s+${INDEX_NAME}\\b`, + 'i', + ) + expect(sql).toMatch(createsIt) + }) + + it('the index is partial on booking_id, so anonymous redemptions are not collapsed together', () => { + const sql = migrationSources() + const stmt = sql.slice(sql.indexOf(`CREATE UNIQUE INDEX IF NOT EXISTS ${INDEX_NAME}`)) + // Without the WHERE clause, every anonymous redemption (booking_id NULL, + // from the property-level /g/[slug] guidebook) would still be distinct in + // Postgres — NULLs never collide — but a later change to NULLS NOT + // DISTINCT would merge DIFFERENT guests into one row. The predicate makes + // the intent explicit rather than resting on NULL semantics. + expect(stmt.slice(0, 400)).toMatch(/WHERE\s+booking_id\s+IS\s+NOT\s+NULL/i) + }) + + it('the route treats 23505 as success rather than reporting it', () => { + const src = readFileSync(join(ROOT, ROUTE), 'utf8') + expect(src).toContain('23505') + // The 23505 check must come BEFORE the generic reportError branch, or a + // duplicate still gets reported. + expect(src.indexOf("'23505'")).toBeLessThan(src.indexOf('route.guidebook.redeem.insert')) + }) + + it('the route names the index it depends on, so a grep for the index finds its consumer', () => { + const src = readFileSync(join(ROOT, ROUTE), 'utf8') + expect(src).toContain(INDEX_NAME) + }) +}) diff --git a/unit/route-handlers/guidebook-redeem.test.ts b/unit/route-handlers/guidebook-redeem.test.ts index a94c82cb..8dbec3c2 100644 --- a/unit/route-handlers/guidebook-redeem.test.ts +++ b/unit/route-handlers/guidebook-redeem.test.ts @@ -4,6 +4,9 @@ import { NextRequest } from 'next/server' vi.mock('@/lib/supabase/server', () => ({ createServiceClient: vi.fn(), })) +vi.mock('@/lib/observability/report-error', () => ({ + reportError: vi.fn(), +})) vi.mock('@/lib/rate-limit', async () => { // checkLimit() is now the only sanctioned way to consult a limiter // (lib/rate-limit.ts). The stub delegates to the limiter doubles below @@ -19,6 +22,7 @@ vi.mock('@/lib/rate-limit', async () => { import { POST } from '@/app/api/guidebook/redeem/route' import { createServiceClient } from '@/lib/supabase/server' import { guidebookRedeemLimiter } from '@/lib/rate-limit' +import { reportError } from '@/lib/observability/report-error' // Real UUIDs, because every one of these ids is a Postgres `uuid` column and // the route now shape-checks before querying. The previous fixtures @@ -225,6 +229,45 @@ describe('POST /api/guidebook/redeem', () => { }) }) + it('treats a 23505 as already-counted-today, not as a failure to report', async () => { + const reportSpy = vi.mocked(reportError) + const service = makeServiceClient({ + insertResult: { + error: { + code: '23505', + message: 'duplicate key value violates unique constraint "uniq_guidebook_offer_redemptions_sponsor_booking_day"', + }, + }, + }) + vi.mocked(createServiceClient).mockReturnValue(service as never) + + const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) + + // Reopening the pass is the normal case — the guest looks at the offer, + // closes it, walks to the business, opens it again for staff. The second + // open must be a no-op, NOT a reported error, or the dedup constraint just + // moves the noise from the sponsor's count into Sentry. + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ ok: true }) + expect(reportSpy).not.toHaveBeenCalled() + }) + + it('still reports a non-23505 insert failure', async () => { + const reportSpy = vi.mocked(reportError) + const service = makeServiceClient({ + insertResult: { error: { code: '42501', message: 'new row violates row-level security policy' } }, + }) + vi.mocked(createServiceClient).mockReturnValue(service as never) + + const res = await POST(postRequest({ sponsorId: SPONSOR_ID })) + + expect(res.status).toBe(200) + expect(reportSpy).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ site: 'route.guidebook.redeem.insert' }), + ) + }) + it('returns ok:true even when the insert fails (table not yet migrated) — never fails the guest UX', async () => { const service = makeServiceClient({ insertResult: { error: { message: 'relation does not exist' } } }) vi.mocked(createServiceClient).mockReturnValue(service as never) From 935583fa333f476c527f80ea1ce20c516b8a91e4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 16:57:58 +0000 Subject: [PATCH 07/11] fix(guidebook): shape-check the media-kit token on all three entry points MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit guidebook_sponsors.media_kit_token is a `uuid`, and all three surfaces that take one only checked `typeof === 'string'`. Postgres does not coerce, so a well-formed-but-not-UUID token is error 22P02 — and each surface turned that into the wrong answer, in opposite directions: - /g/kit/[media_kit_token] and its /print sibling already carried the note that a failed READ must not read as an invalid token, and throw to the error boundary rather than notFound() for exactly that reason. The inverse held just as strongly and was unhandled: a plainly invalid URL 22P02'd, unwrap() threw, and the sponsor got "something went wrong on our side" for a bad link. Both failures now get their own honest surface — malformed 404s before any database round trip, a real query failure still reaches the boundary. - /api/guidebook/sponsor-checkout passed the token straight through to createSponsorCheckoutSession, where it 22P02'd out of unwrap() into the action's catch — reporting to Sentry and telling the sponsor "Unable to start checkout. Please try again." for a link that will never work however many times they try. It now returns the same "Invalid media kit link." a nonexistent token gets, without touching the DB or Sentry. All three are public and unauthenticated, so the Sentry-report-per-malformed- request path was also a free way to burn the quota and bury these surfaces' genuine failures in noise. Same fixture problem as the redeem route, and worth repeating because it is a pattern: this suite used 'kit-token-abc-123', which could not have existed in production — against the live schema it is 22P02, not a miss. The new page tests use a double that answers 22P02 for a non-UUID compared to a uuid column, so they fail when the guard is removed rather than passing regardless. All three guards canary-verified individually. The media-kit pages had no test file at all before this. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- app/api/guidebook/sponsor-checkout/route.ts | 13 ++- app/g/kit/[media_kit_token]/page.tsx | 10 ++ app/g/kit/[media_kit_token]/print/page.tsx | 7 ++ unit/pages/media-kit-token-shape.test.ts | 93 +++++++++++++++++++ .../guidebook-sponsor-checkout.test.ts | 39 ++++++-- 5 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 unit/pages/media-kit-token-shape.test.ts diff --git a/app/api/guidebook/sponsor-checkout/route.ts b/app/api/guidebook/sponsor-checkout/route.ts index 64d96ac4..de7989ca 100644 --- a/app/api/guidebook/sponsor-checkout/route.ts +++ b/app/api/guidebook/sponsor-checkout/route.ts @@ -3,6 +3,7 @@ import { createSponsorCheckoutSession } from '@/app/actions/guidebook' import { guidebookSponsorCheckoutLimiter, checkLimit } from '@/lib/rate-limit' import { extractClientIp } from '@/lib/integrations/webhook-verification' import { reportError } from '@/lib/observability/report-error' +import { isUuid } from '@/lib/validation/uuid' export async function POST(req: NextRequest): Promise { // Throttle BEFORE the token is read, so a guessing attack is capped by the @@ -31,9 +32,17 @@ export async function POST(req: NextRequest): Promise { try { const body = await req.json() as { mediaKitToken?: string } - if (!body.mediaKitToken || typeof body.mediaKitToken !== 'string') { + // Shape-checked, not just type-checked. guidebook_sponsors.media_kit_token + // is a `uuid`, so a malformed token reaches `.eq()` as Postgres 22P02, + // throws out of the action's unwrap(), and lands in its catch — which + // reports to Sentry and tells the sponsor "Unable to start checkout. + // Please try again." for a link that will never work no matter how many + // times they try. On a public unauthenticated endpoint that is also a free + // way to burn the Sentry quota. An unusable token is an invalid link, and + // gets the same message a nonexistent one does. + if (!isUuid(body.mediaKitToken)) { return NextResponse.json( - { error: 'mediaKitToken is required' }, + { error: 'Invalid media kit link.' }, { status: 400 } ) } diff --git a/app/g/kit/[media_kit_token]/page.tsx b/app/g/kit/[media_kit_token]/page.tsx index 33c24376..45cb0a17 100644 --- a/app/g/kit/[media_kit_token]/page.tsx +++ b/app/g/kit/[media_kit_token]/page.tsx @@ -2,6 +2,7 @@ import { unwrap } from '@/lib/supabase/unwrap' import { notFound } from 'next/navigation' import { Archivo, Source_Serif_4 } from 'next/font/google' import { createServiceClient } from '@/lib/supabase/server' +import { isUuid } from '@/lib/validation/uuid' import { MediaKitClient } from './media-kit-client' import type { GuidebookSponsor } from '@/types/database' @@ -25,6 +26,15 @@ export default async function MediaKitPage({ params: Promise<{ media_kit_token: string }> }) { const { media_kit_token } = await params + + // media_kit_token is a `uuid` column, so a malformed one is Postgres 22P02 — + // which unwrap() turns into a throw and the segment error boundary renders + // as "something went wrong". That is the exact inversion of the note below: + // an outage must not read as an invalid token, but neither may a genuinely + // invalid token read as an outage. Shape-check first and 404 it, so each of + // the two failures gets its own honest surface. + if (!isUuid(media_kit_token)) notFound() + const supabase = createServiceClient({ publicSurface: 'g-kit--media-kit-token-' }) // Token-gated public page: a failed read used to fall into notFound(), diff --git a/app/g/kit/[media_kit_token]/print/page.tsx b/app/g/kit/[media_kit_token]/print/page.tsx index f3dbb600..b1820d85 100644 --- a/app/g/kit/[media_kit_token]/print/page.tsx +++ b/app/g/kit/[media_kit_token]/print/page.tsx @@ -1,6 +1,7 @@ import { notFound } from 'next/navigation' import { Archivo, Source_Serif_4 } from 'next/font/google' import { createServiceClient } from '@/lib/supabase/server' +import { isUuid } from '@/lib/validation/uuid' import { unwrap } from '@/lib/supabase/unwrap' import { PrintKit } from './print-kit' import type { GuidebookSponsor } from '@/types/database' @@ -25,6 +26,12 @@ export default async function PrintKitPage({ params: Promise<{ media_kit_token: string }> }) { const { media_kit_token } = await params + + // Same shape check as the non-print page — media_kit_token is a `uuid`, so a + // malformed one is 22P02, which unwrap() escalates to the error boundary + // rather than the 404 an invalid link deserves. + if (!isUuid(media_kit_token)) notFound() + const supabase = createServiceClient({ publicSurface: 'g-kit--media-kit-token-print' }) const sponsorRes = await supabase diff --git a/unit/pages/media-kit-token-shape.test.ts b/unit/pages/media-kit-token-shape.test.ts new file mode 100644 index 00000000..97225ebf --- /dev/null +++ b/unit/pages/media-kit-token-shape.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('next/navigation', () => ({ + notFound: vi.fn(() => { throw new Error('NEXT_NOT_FOUND') }), +})) +vi.mock('@/lib/supabase/server', () => ({ + createServiceClient: vi.fn(), +})) +// next/font/google runs a real font fetch at import time in this environment. +vi.mock('next/font/google', () => ({ + Archivo: () => ({ variable: 'archivo' }), + Source_Serif_4: () => ({ variable: 'serif' }), +})) +vi.mock('@/app/g/kit/[media_kit_token]/media-kit-client', () => ({ + MediaKitClient: () => null, +})) +vi.mock('@/app/g/kit/[media_kit_token]/print/print-kit', () => ({ + PrintKit: () => null, +})) + +import MediaKitPage from '@/app/g/kit/[media_kit_token]/page' +import PrintKitPage from '@/app/g/kit/[media_kit_token]/print/page' +import { createServiceClient } from '@/lib/supabase/server' +import { notFound } from 'next/navigation' + +const VALID_TOKEN = '66666666-6666-4666-8666-666666666666' + +/** + * Answers 22P02 for a non-UUID compared against a `uuid` column, because that + * is what Postgres does — it does not return zero rows. A double that quietly + * accepts anything makes a malformed-input test pass whether the handling + * exists or not. + */ +const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + +function makeSupabase() { + const from = vi.fn(() => { + const eqArgs: [string, unknown][] = [] + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const chain: any = {} + chain.select = vi.fn(() => chain) + chain.eq = vi.fn((col: string, val: unknown) => { eqArgs.push([col, val]); return chain }) + chain.maybeSingle = vi.fn(() => { + const bad = eqArgs.find(([col, val]) => + (col === 'media_kit_token' || col === 'id') && !UUID_RE.test(String(val))) + if (bad) { + return Promise.resolve({ + data: null, + error: { code: '22P02', message: `invalid input syntax for type uuid: "${String(bad[1])}"` }, + }) + } + return Promise.resolve({ data: null, error: null }) + }) + return chain + }) + return { from } +} + +const pages: [string, (a: { params: Promise<{ media_kit_token: string }> }) => Promise][] = [ + ['media kit', MediaKitPage as never], + ['print media kit', PrintKitPage as never], +] + +describe.each(pages)('/g/kit — %s page token shape', (_label, Page) => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(createServiceClient).mockReturnValue(makeSupabase() as never) + }) + + it('404s a malformed token instead of rendering an outage', async () => { + // The intent already documented on these pages is that a failed READ must + // not read as an invalid token. The inverse held just as strongly and was + // not handled: media_kit_token is a `uuid`, so a malformed one is 22P02, + // which unwrap() escalates into the segment error boundary — telling a + // sponsor with a plainly bad URL that something went wrong on our side. + await expect( + Page({ params: Promise.resolve({ media_kit_token: 'kit-token-abc-123' }) }) + ).rejects.toThrow('NEXT_NOT_FOUND') + + expect(notFound).toHaveBeenCalled() + // Cheap and load-bearing: no database round trip for input that could + // never match, on a public unauthenticated surface. + expect(createServiceClient).not.toHaveBeenCalled() + }) + + it('still queries — and 404s — for a well-formed token that matches no sponsor', async () => { + await expect( + Page({ params: Promise.resolve({ media_kit_token: VALID_TOKEN }) }) + ).rejects.toThrow('NEXT_NOT_FOUND') + + expect(createServiceClient).toHaveBeenCalled() + }) +}) diff --git a/unit/route-handlers/guidebook-sponsor-checkout.test.ts b/unit/route-handlers/guidebook-sponsor-checkout.test.ts index e7a18def..3ea61952 100644 --- a/unit/route-handlers/guidebook-sponsor-checkout.test.ts +++ b/unit/route-handlers/guidebook-sponsor-checkout.test.ts @@ -28,6 +28,13 @@ function postRequest(body: unknown) { }) } +// Real UUIDs: guidebook_sponsors.media_kit_token is a `uuid` column, so the +// previous fixture ('kit-token-abc-123') could not have existed in production +// — against the live schema it is error 22P02, not a miss. The suite was green +// on input the database would have rejected outright. +const KIT_TOKEN = '66666666-6666-4666-8666-666666666666' +const MISSING_TOKEN = '77777777-7777-4777-8777-777777777777' + describe('POST /api/guidebook/sponsor-checkout', () => { beforeEach(() => { vi.clearAllMocks() @@ -37,7 +44,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('returns 429 without touching the checkout action when the limiter denies', async () => { vi.mocked(guidebookSponsorCheckoutLimiter.limit).mockResolvedValue({ success: false } as never) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) const json = await res.json() expect(res.status).toBe(429) @@ -56,7 +63,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { return { url: 'https://checkout.stripe.com/pay/cs_1' } }) - await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + await POST(postRequest({ mediaKitToken: KIT_TOKEN })) expect(callOrder).toEqual(['limit', 'lookup']) }) @@ -72,7 +79,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { 'x-real-ip': '203.0.113.7', 'x-forwarded-for': '1.2.3.4, 203.0.113.7', }, - body: JSON.stringify({ mediaKitToken: 'kit-token-abc-123' }), + body: JSON.stringify({ mediaKitToken: KIT_TOKEN }), }) vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ url: 'https://checkout.stripe.com/pay/cs_1' }) @@ -96,18 +103,32 @@ describe('POST /api/guidebook/sponsor-checkout', () => { expect(createSponsorCheckoutSession).not.toHaveBeenCalled() }) + it('rejects a malformed mediaKitToken as an invalid link, without reaching the action', async () => { + const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const json = await res.json() + + // media_kit_token is a uuid. A well-formed-but-not-UUID string used to + // sail past the typeof check into the action's `.eq()`, hit 22P02, throw + // out of unwrap(), and land in the action's catch — which reports to + // Sentry and tells the sponsor "Unable to start checkout. Please try + // again." for a link that will never work however many times they try. + expect(res.status).toBe(400) + expect(json).toEqual({ error: 'Invalid media kit link.' }) + expect(createSponsorCheckoutSession).not.toHaveBeenCalled() + }) + it('passes the exact client-supplied mediaKitToken through to the action unmodified — the action itself is the only place org scoping happens (media_kit_token lookup)', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ url: 'https://checkout.stripe.com/pay/cs_1' }) - await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + await POST(postRequest({ mediaKitToken: KIT_TOKEN })) - expect(createSponsorCheckoutSession).toHaveBeenCalledWith('kit-token-abc-123') + expect(createSponsorCheckoutSession).toHaveBeenCalledWith(KIT_TOKEN) }) it('returns the checkout URL on success', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ url: 'https://checkout.stripe.com/pay/cs_1' }) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) const json = await res.json() expect(res.status).toBe(200) @@ -117,7 +138,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('surfaces an action-level error (e.g. invalid/unknown token) as a 400, not a 500', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ error: 'Invalid media kit link.' }) - const res = await POST(postRequest({ mediaKitToken: 'nonexistent-token' })) + const res = await POST(postRequest({ mediaKitToken: MISSING_TOKEN })) const json = await res.json() expect(res.status).toBe(400) @@ -127,7 +148,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('returns a generic 500 (no raw error detail) when the action throws unexpectedly', async () => { vi.mocked(createSponsorCheckoutSession).mockRejectedValue(new Error('stripe network timeout')) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) const json = await res.json() expect(res.status).toBe(500) @@ -137,7 +158,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('returns 400 when the sponsorship slot is already active (a real auth-model rejection for this token route)', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ error: 'This sponsorship slot is already active.' }) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) expect(res.status).toBe(400) }) From 45d5a1115122cc19361757e1533537c3fdbf25f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 17:26:07 +0000 Subject: [PATCH 08/11] feat(guidebook): open_count for engagement; arrival SMS on check-in day MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things you asked for after the dedup landed. ── Engagement counter ─────────────────────────────────────────────────────── The per-day dedup made the redemption count honest but discarded a real second signal: how many times the pass was actually opened. "12 redemptions, opened 31 times" says something "12 redemptions" alone does not. guidebook_offer_redemptions.open_count, incremented on the existing row rather than added as a second table — the dedup key IS the natural grain, so the counter stays bounded by the same constraint instead of growing per tap, and both numbers come out of one query: COUNT(*) redemptions, SUM(open_count) opens. The write moves to record_guidebook_offer_open(). It has to be a function: the arbiter is a partial EXPRESSION index and PostgREST's on_conflict takes only plain column names, so the upsert is not expressible through the JS client at all — and read-then-write in the route would be a TOCTOU where two taps racing both read 1 and both write 2. p_booking_id is DEFAULT NULL so the anonymous case can omit it; without that the generated Args type is a required non-nullable string and the only ways to pass null are a cast or a second write path. Postgres grants EXECUTE to PUBLIC by default, which on Supabase means anon could call it over /rest/v1/rpc/ with the publishable key and write rows to a tenant table with no session. Revoked and granted to service_role only — verified against the live DB, not just asserted in the file. The guardrail now covers that, the ON CONFLICT arbiter matching the index, and the route not regressing to a bare insert (which would leave open_count pinned at 1 and flatline engagement silently). All three canary-verified. ── Arrival reminder ───────────────────────────────────────────────────────── The morning cron's filter is `checkin_date <= today AND checkout_date >= today`, so a guest whose stay STARTS today is included — but it fires 7-11 AM and check-in is typically mid-afternoon. They were getting the full nudge ("it's 72°F at your rental, here's a coffee spot 0.4 mi away") hours before they had keys. New arrival_reminder template, sent instead of the nudge on check-in day, via the same claim slot so a guest still gets exactly one morning message. Placed before the lat/lng and weather guards — an arrival reminder needs neither, and a property with no coordinates should still be able to send one. The check-in sentence is omitted entirely rather than left blank when checkin_time is null (27 of 27 production properties have one, but OwnerRez-synced properties write null explicitly). The outgoing guest on a same-day flip is deliberately unchanged, per your call: `checkout_date >= today` keeps them on checkout morning, when they ARE still in the house. formatTime12h moves out of the guest guidebook Client Component to lib/utils/time-of-day.ts so the Inngest step can use it — these are wall-clock values with no timezone, so they must not be run through Intl with a timeZone. Migration applied to production and E2E. Note: MCP apply_migration assigned its own version, and a DIFFERENT one per project (…170639 vs …170648), matching neither the committed filename — exactly the ledger drift CLAUDE.md documents. Both ledgers normalized to the file's version. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- app/api/guidebook/redeem/route.ts | 40 +++++----- components/guidebook/guest-guidebook-view.tsx | 12 +-- .../functions/guidebook-sms-morning-cron.ts | 46 ++++++++++- lib/sms/template-registry.ts | 12 +++ lib/sms/templates.ts | 10 +++ lib/utils/time-of-day.ts | 28 +++++++ ...60807170000_guidebook_offer_open_count.sql | 63 +++++++++++++++ types/database.generated.ts | 7 ++ types/database.ts | 7 ++ .../redemption-dedup-pairing.test.ts | 74 ++++++++++++----- .../guidebook-sms-morning-cron.test.ts | 80 +++++++++++++++++++ unit/lib/sms-template-registry.test.ts | 3 +- unit/route-handlers/guidebook-redeem.test.ts | 74 +++++++---------- 13 files changed, 359 insertions(+), 97 deletions(-) create mode 100644 lib/utils/time-of-day.ts create mode 100644 supabase/migrations/20260807170000_guidebook_offer_open_count.sql diff --git a/app/api/guidebook/redeem/route.ts b/app/api/guidebook/redeem/route.ts index 1dd3e054..05ea035e 100644 --- a/app/api/guidebook/redeem/route.ts +++ b/app/api/guidebook/redeem/route.ts @@ -77,36 +77,40 @@ export async function POST(req: NextRequest): Promise { if (booking && booking.org_id === sponsor.org_id) bookingId = booking.id } - const { error } = await supabase.from('guidebook_offer_redemptions').insert({ - org_id: sponsor.org_id, - sponsor_id: sponsor.id, - booking_id: bookingId, - }) - - // 23505 against uniq_guidebook_offer_redemptions_sponsor_booking_day - // (migration 20260807150000) is the SUCCESS path, not a failure: this - // sponsor is already counted for this booking today. + // Insert-or-increment, as ONE statement (migration 20260807170000). // // The row is written when the guest opens the redemption pass — the coupon // they show at the counter — and opening it more than once is the NORMAL // case: look at the offer from the couch, close it, walk over, open it - // again for staff. Counting each reopen inflated the exact number a paying - // sponsor judges their slot by. + // again for staff. That gives two genuinely different numbers, and the + // sponsor wants both: COUNT(*) is redemptions (deduped per booking per day + // by uniq_guidebook_offer_redemptions_sponsor_booking_day), SUM(open_count) + // is engagement. // // Per day, not per stay: a daily perk is legitimately redeemable on each // day of a booking, so collapsing the whole stay would under-count. // - // Caught here rather than expressed as upsert({onConflict}): the index is - // an EXPRESSION index (on the UTC date of opened_at), and PostgREST's - // on_conflict only takes plain column names. + // An RPC rather than the JS client, for two reasons. The arbiter is a + // PARTIAL EXPRESSION index (on the UTC date of opened_at) and PostgREST's + // on_conflict takes only plain column names, so this upsert is not + // expressible through the client at all. And read-then-write here would be + // a TOCTOU — two taps racing would both read 1 and both write 2. // // Anonymous redemptions (bookingId null, from the property-level // /g/[slug] guidebook) fall outside the partial index by design — with no // guest identity there is nothing to dedupe on, and collapsing them by - // (sponsor, day) would merge different guests into one. - if (error?.code === '23505') { - return NextResponse.json({ ok: true }) - } + // (sponsor, day) would merge different guests into one. Each is its own + // row at open_count = 1, so both aggregates still read correctly. + // p_booking_id is OMITTED rather than passed as null for the anonymous + // case — the function declares `DEFAULT NULL` precisely so this is + // expressible. Supabase's type generator has no notion of a nullable + // argument, so without the default the only ways to call this with a null + // would be a cast or a second write path. + const { error } = await supabase.rpc('record_guidebook_offer_open', { + p_org_id: sponsor.org_id, + p_sponsor_id: sponsor.id, + ...(bookingId ? { p_booking_id: bookingId } : {}), + }) if (error) { // Redemption logging is best-effort — never fail the guest UX over it — diff --git a/components/guidebook/guest-guidebook-view.tsx b/components/guidebook/guest-guidebook-view.tsx index fe60672f..08e11700 100644 --- a/components/guidebook/guest-guidebook-view.tsx +++ b/components/guidebook/guest-guidebook-view.tsx @@ -8,6 +8,7 @@ import { getActiveSlotTypes, getTimeOfDay } from '@/lib/weather/tomorrow' import { formatOffer } from '@/lib/guidebook/offer' import { CopyButton } from './copy-button' import styles from './guest-guidebook-view.module.css' +import { formatTime12h } from '@/lib/utils/time-of-day' const CHARCOAL = '#0E0E10' const CARD = '#17171A' @@ -45,17 +46,6 @@ const WHY_CHIP: Record = { other: "Today's pick", } -function formatTime12h(time: string | null | undefined): string | null { - if (!time) return null - const [hourStr, minuteStr] = time.split(':') - const hour = Number(hourStr) - const minute = Number(minuteStr) - if (Number.isNaN(hour) || Number.isNaN(minute)) return null - const period = hour >= 12 ? 'PM' : 'AM' - const displayHour = hour % 12 === 0 ? 12 : hour % 12 - return `${displayHour}:${minute.toString().padStart(2, '0')} ${period}` -} - function formatClock(d: Date): string { let hour = d.getHours() const minute = d.getMinutes().toString().padStart(2, '0') diff --git a/lib/inngest/functions/guidebook-sms-morning-cron.ts b/lib/inngest/functions/guidebook-sms-morning-cron.ts index 796732dc..d9e81138 100644 --- a/lib/inngest/functions/guidebook-sms-morning-cron.ts +++ b/lib/inngest/functions/guidebook-sms-morning-cron.ts @@ -6,6 +6,7 @@ import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' import { sendClaimedDailySms } from '@/lib/sms/optin-claim' +import { formatTime12h } from '@/lib/utils/time-of-day' import { pickNearestSponsor } from '@/lib/sms/pick-nearest-sponsor' import { unwrapJoin } from '@/lib/utils/supabase-joins' import { getFeaturedAmenityLine } from '@/lib/guidebook/featured-amenities' @@ -141,10 +142,51 @@ export const guidebookSmsMorningSend = inngest.createFunction( const { data: property } = await supabase .from('properties') - .select('id, name, lat, lng, amenities') + .select('id, name, lat, lng, amenities, checkin_time') .eq('id', propertyId) .maybeSingle() - if (!property?.lat || !property?.lng) return false + if (!property) return false + + // ── Check-in day: this guest has not arrived yet ────────────────────── + // + // The eligibility filter is `checkin_date <= today AND checkout_date >= + // today`, so a guest whose stay STARTS today is in the set — but this + // cron fires 7-11 AM and check-in is typically mid-afternoon. They were + // getting the full nudge — "it's 72°F at your rental, here's a coffee + // spot 0.4 mi away" — hours before they had keys, as though they were + // already in the house. + // + // Deliberately BEFORE the lat/lng and weather guards below: an arrival + // reminder needs neither, and a property without coordinates should + // still be able to send one. + // + // The outgoing guest on a same-day flip is a different case and is left + // alone: `checkout_date >= today` includes them on checkout morning, + // when they ARE still in the house and a local recommendation still + // lands. The evening cron already excludes them (`checkout_date > + // today`). + // + // Same claim slot, so a guest still gets exactly one morning message — + // this one instead of the nudge, never both. + if (checkinDate === todayDate) { + return await sendClaimedDailySms( + supabase, optinId, 'last_morning_sms_date', todayDate, + async () => { + const checkinAt = formatTime12h(property.checkin_time) + const arrivalBody = await renderSmsBody(orgId, 'arrival_reminder', { + property_name: property.name, + // Omitted entirely rather than left blank when the property has + // no check-in time — 27 of 27 production properties have one, + // but the column is nullable and OwnerRez-synced properties + // explicitly write null. + checkin_line: checkinAt ? `Just a reminder that check-in is at ${checkinAt}.` : '', + }) + return await sendSMS(optin.phone_e164, arrivalBody, { category: 'nudge', orgId }) + }, + ) + } + + if (!property.lat || !property.lng) return false // Featured-amenity content is independent of sponsors — a property // with no active sponsors can still get a message if it has featured diff --git a/lib/sms/template-registry.ts b/lib/sms/template-registry.ts index b879153a..84d4722f 100644 --- a/lib/sms/template-registry.ts +++ b/lib/sms/template-registry.ts @@ -13,6 +13,7 @@ export type SmsTemplateKey = | 'door_code' | 'morning_nudge' + | 'arrival_reminder' | 'evening_nudge' | 'rain_alert' | 'stay_extension' @@ -88,6 +89,17 @@ export const SMS_TEMPLATE_REGISTRY: SmsTemplateConfig[] = [ ], defaultBody: 'Good morning! It\'s {{temperature}}°F at {{property_name}} today. {{offer_line}} Reply STOP to opt out.', }, + { + key: 'arrival_reminder', + label: 'Arrival Reminder — Check-In Day', + description: 'Replaces the morning nudge on a guest\'s CHECK-IN day. The morning cron runs 7-11 AM but check-in is typically mid-afternoon, so a guest arriving today would otherwise get "it\'s 72°F at your rental, here\'s a coffee spot" hours before they have keys.', + audience: 'guest', + variables: [ + { token: '{{property_name}}', description: 'Property name', example: 'Lakeside Lodge' }, + { token: '{{checkin_line}}', description: 'Check-in time sentence — empty when the property has no check-in time set, so the message still reads correctly', example: 'Just a reminder that check-in is at 4:00 PM.' }, + ], + defaultBody: 'Looking forward to hosting you at {{property_name}} today! {{checkin_line}} Reply STOP to opt out.', + }, { key: 'evening_nudge', label: 'Evening Nudge — Guest Stay', diff --git a/lib/sms/templates.ts b/lib/sms/templates.ts index 344bc491..31ec9f50 100644 --- a/lib/sms/templates.ts +++ b/lib/sms/templates.ts @@ -103,6 +103,16 @@ function renderDefault( vars.offer_line ? String(vars.offer_line) : null ) + case 'arrival_reminder': + // Inline rather than a telnyx.ts builder: there is no legacy builder to + // preserve, and checkin_line is already assembled (or omitted) by the + // caller so a property with no check-in time still reads correctly. + return [ + `Looking forward to hosting you at ${vars.property_name ?? 'your rental'} today!`, + vars.checkin_line ? String(vars.checkin_line) : '', + 'Reply STOP to opt out.', + ].filter(Boolean).join(' ') + case 'evening_nudge': return buildEveningNudgeSMS( String(vars.property_name ?? ''), diff --git a/lib/utils/time-of-day.ts b/lib/utils/time-of-day.ts new file mode 100644 index 00000000..6e965027 --- /dev/null +++ b/lib/utils/time-of-day.ts @@ -0,0 +1,28 @@ +// lib/utils/time-of-day.ts +// +// Formatting for a bare `time without time zone` column (properties.checkin_time +// / checkout_time), which arrives from PostgREST as "16:00:00". +// +// Deliberately NOT `import 'server-only'`: a pure string transform with no +// dependencies, used by the guest guidebook (a Client Component) and by the +// morning SMS send (an Inngest step). Same rationale as lib/validation/uuid.ts. +// +// Note these values carry NO timezone — they are wall-clock times at the +// property, which is exactly what a guest wants read back to them ("check-in +// is at 4:00 PM"). Do not run them through Intl with a timeZone; that would +// reinterpret a wall-clock string as an instant and shift it. + +/** + * "16:00:00" → "4:00 PM". Returns null for null/empty/unparseable input so + * callers can omit the phrase entirely rather than render "check-in is at ." + */ +export function formatTime12h(time: string | null | undefined): string | null { + if (!time) return null + const [hourStr, minuteStr] = time.split(':') + const hour = Number(hourStr) + const minute = Number(minuteStr) + if (Number.isNaN(hour) || Number.isNaN(minute)) return null + const period = hour >= 12 ? 'PM' : 'AM' + const displayHour = hour % 12 === 0 ? 12 : hour % 12 + return `${displayHour}:${minute.toString().padStart(2, '0')} ${period}` +} diff --git a/supabase/migrations/20260807170000_guidebook_offer_open_count.sql b/supabase/migrations/20260807170000_guidebook_offer_open_count.sql new file mode 100644 index 00000000..befc5b4c --- /dev/null +++ b/supabase/migrations/20260807170000_guidebook_offer_open_count.sql @@ -0,0 +1,63 @@ +-- guidebook_offer_redemptions.open_count — engagement, alongside the deduped +-- redemption it lives on. +-- +-- 20260807150000 made the row unique per (sponsor, booking, UTC day) so a paying +-- sponsor's redemption count stops being inflated by a guest reopening their own +-- coupon. Correct for that number, but it threw away a real second signal: how +-- many times the pass was actually opened. "12 redemptions, opened 31 times" +-- says something "12 redemptions" alone does not. +-- +-- One column on the existing row rather than a second table: the dedup key IS +-- the natural grain for the counter, so the count stays bounded by the same +-- constraint instead of growing per tap, and both numbers come out of one +-- query — COUNT(*) for redemptions, SUM(open_count) for opens. +-- +-- Anonymous redemptions (booking_id NULL, property-level /g/[slug]) sit outside +-- the partial unique index, so each open is its own row at open_count = 1. Both +-- aggregates still read correctly there; they simply carry no more information +-- than each other, which is the honest answer when there is no guest identity +-- to attribute opens to. + +ALTER TABLE guidebook_offer_redemptions + ADD COLUMN IF NOT EXISTS open_count integer NOT NULL DEFAULT 1; + +COMMENT ON COLUMN guidebook_offer_redemptions.open_count IS + 'Times the redemption pass was opened for this (sponsor, booking, UTC day). COUNT(*) = redemptions; SUM(open_count) = opens.'; + +-- The insert-or-increment, as one statement. +-- +-- It has to be a function: the arbiter is a PARTIAL EXPRESSION index, and +-- PostgREST's on_conflict parameter only accepts plain column names, so the +-- JS client cannot express this upsert at all. Doing it as read-then-write in +-- the route would be a TOCTOU — two taps racing would both read 1 and both +-- write 2. +-- +-- SECURITY INVOKER (the default), deliberately. The only caller is the redeem +-- route holding the service role, which already bypasses RLS; a DEFINER +-- function here would add an privilege-escalation surface that buys nothing. +CREATE OR REPLACE FUNCTION public.record_guidebook_offer_open( + p_org_id uuid, + p_sponsor_id uuid, + -- DEFAULT NULL so the anonymous case (property-level /g/[slug], no booking + -- token) can omit the argument entirely. Without a default, Supabase's type + -- generator emits `p_booking_id: string` — required and non-nullable — and + -- the only ways to call it with a null are a cast or a second write path. + p_booking_id uuid DEFAULT NULL +) RETURNS void +LANGUAGE sql +SET search_path = public +AS $$ + INSERT INTO guidebook_offer_redemptions (org_id, sponsor_id, booking_id) + VALUES (p_org_id, p_sponsor_id, p_booking_id) + ON CONFLICT (sponsor_id, booking_id, ((opened_at AT TIME ZONE 'UTC')::date)) + WHERE booking_id IS NOT NULL + DO UPDATE SET open_count = guidebook_offer_redemptions.open_count + 1; +$$; + +-- Postgres grants EXECUTE on a new function to PUBLIC by default, which on a +-- Supabase project means anon can call it over /rest/v1/rpc/ with the +-- publishable key — i.e. write rows to a tenant table with no session at all. +-- Every anon TABLE grant was revoked on 2026-07-24; a function is the same +-- exposure through a different door. +REVOKE ALL ON FUNCTION public.record_guidebook_offer_open(uuid, uuid, uuid) FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.record_guidebook_offer_open(uuid, uuid, uuid) TO service_role; diff --git a/types/database.generated.ts b/types/database.generated.ts index 2bb412d9..8ad88d50 100644 --- a/types/database.generated.ts +++ b/types/database.generated.ts @@ -1272,6 +1272,7 @@ export type Database = { Row: { booking_id: string | null id: string + open_count: number opened_at: string org_id: string sponsor_id: string @@ -1279,6 +1280,7 @@ export type Database = { Insert: { booking_id?: string | null id?: string + open_count?: number opened_at?: string org_id: string sponsor_id: string @@ -1286,6 +1288,7 @@ export type Database = { Update: { booking_id?: string | null id?: string + open_count?: number opened_at?: string org_id?: string sponsor_id?: string @@ -5820,6 +5823,10 @@ export type Database = { Returns: string } recompute_vendor_scores: { Args: never; Returns: number } + record_guidebook_offer_open: { + Args: { p_booking_id?: string; p_org_id: string; p_sponsor_id: string } + Returns: undefined + } remove_crew_from_turnover: { Args: { p_crew_member_id: string diff --git a/types/database.ts b/types/database.ts index faad49d9..1a60cfaa 100644 --- a/types/database.ts +++ b/types/database.ts @@ -1480,6 +1480,13 @@ export interface GuidebookOfferRedemption { sponsor_id: string booking_id: string | null opened_at: string + /** + * Times the pass was opened for this (sponsor, booking, UTC day). The row + * itself is deduped by uniq_guidebook_offer_redemptions_sponsor_booking_day, + * so COUNT(*) is redemptions and SUM(open_count) is engagement — two numbers + * a sponsor wants separately. NOT NULL DEFAULT 1 (20260807170000). + */ + open_count: number } export interface GuidebookGuestSmsOptin { diff --git a/unit/guardrails/redemption-dedup-pairing.test.ts b/unit/guardrails/redemption-dedup-pairing.test.ts index cfb3a081..3e630197 100644 --- a/unit/guardrails/redemption-dedup-pairing.test.ts +++ b/unit/guardrails/redemption-dedup-pairing.test.ts @@ -1,26 +1,29 @@ // unit/guardrails/redemption-dedup-pairing.test.ts // -// guidebook_offer_redemptions has a dedup constraint and a handler branch that -// depends on it, and they live in different files with nothing tying them -// together. +// guidebook_offer_redemptions carries TWO sponsor-facing numbers out of one +// row — COUNT(*) is redemptions, SUM(open_count) is engagement — and both +// depend on a unique index, an insert-or-increment function, and a route that +// calls it, spread across three files with nothing tying them together. // -// The row is written when a guest opens the redemption pass — the coupon they -// show at the counter — and reopening it is the normal case, so raw COUNT(*) -// overstates real redemptions by however many times the guest looked at their -// own coupon. That count is what a paying sponsor judges their slot by. +// The row is written when a guest opens the redemption pass, the coupon they +// show at the counter. Reopening it is the normal case (look at the offer, +// close it, walk over, open it again for staff), which is precisely why the +// two numbers differ and why a sponsor wants both. // -// Two ways the pairing silently breaks: +// Every way this breaks is silent, and none of them shows up as a test failure +// anywhere else — no unit test touches the real database, and the sponsor +// report that would surface a wrong number does not exist yet: // -// - the unique index goes away (migration reverted, index renamed, table -// rebuilt) and the route's 23505 branch becomes dead code. Nothing errors; -// the counts just quietly inflate again. -// - the route stops handling 23505 and every reopen becomes a reported -// error instead — the noise moves from the sponsor's number into Sentry, -// which is not an improvement. +// - the unique index goes away (migration reverted, renamed, table rebuilt): +// redemptions quietly re-inflate to one row per tap. +// - the function's ON CONFLICT arbiter stops matching the index expression: +// same inflation, one layer down, where the index still looks present. +// - the route swaps the RPC for a plain .insert(): rows stay deduped, but +// open_count sticks at 1 and engagement silently flatlines. +// - the function keeps Postgres's default EXECUTE-to-PUBLIC grant: anon can +// write rows to a tenant table over /rest/v1/rpc/ with no session at all. // -// Neither shows up as a test failure anywhere else: no unit test exercises the -// real database, and the sponsor-facing report that would surface a wrong -// number does not exist yet. This asserts the two halves stay together. +// This asserts all four stay closed. import { describe, it, expect } from 'vitest' import { readFileSync, readdirSync } from 'node:fs' @@ -30,6 +33,7 @@ const ROOT = join(__dirname, '..', '..') const ROUTE = 'app/api/guidebook/redeem/route.ts' const MIGRATIONS_DIR = join(ROOT, 'supabase', 'migrations') const INDEX_NAME = 'uniq_guidebook_offer_redemptions_sponsor_booking_day' +const RPC_NAME = 'record_guidebook_offer_open' function migrationSources(): string { return readdirSync(MIGRATIONS_DIR) @@ -59,12 +63,38 @@ describe('guidebook redemption dedup — constraint and handler stay paired', () expect(stmt.slice(0, 400)).toMatch(/WHERE\s+booking_id\s+IS\s+NOT\s+NULL/i) }) - it('the route treats 23505 as success rather than reporting it', () => { + it('a migration defines the insert-or-increment function against that same index', () => { + const sql = migrationSources() + expect(sql).toContain(`FUNCTION public.${RPC_NAME}`) + // The ON CONFLICT arbiter must match the index expression, or the function + // silently loses its conflict target and every open inserts a new row — + // the exact inflation the index exists to prevent, reintroduced one layer + // down where the index still looks present. + const fn = sql.slice(sql.indexOf(`FUNCTION public.${RPC_NAME}`)) + expect(fn).toMatch(/ON CONFLICT[\s\S]{0,200}opened_at AT TIME ZONE 'UTC'/i) + expect(fn).toMatch(/DO UPDATE SET open_count = [\s\S]{0,80}open_count \+ 1/i) + }) + + it('the function is not executable by anon or authenticated', () => { + const sql = migrationSources() + const fn = sql.slice(sql.indexOf(`FUNCTION public.${RPC_NAME}`)) + // Postgres grants EXECUTE to PUBLIC by default, which on Supabase means + // anon can call it over /rest/v1/rpc/ with the publishable key — writing + // rows to a tenant table with no session at all. Every anon TABLE grant + // was revoked on 2026-07-24; a function is the same exposure via a + // different door. + expect(fn).toMatch(/REVOKE ALL ON FUNCTION[\s\S]{0,200}FROM PUBLIC/i) + expect(fn).toMatch(/GRANT EXECUTE ON FUNCTION[\s\S]{0,200}TO service_role/i) + expect(fn).not.toMatch(/GRANT EXECUTE[\s\S]{0,200}TO (anon|authenticated)/i) + }) + + it('the route writes through the function, not a bare insert it could drift from', () => { const src = readFileSync(join(ROOT, ROUTE), 'utf8') - expect(src).toContain('23505') - // The 23505 check must come BEFORE the generic reportError branch, or a - // duplicate still gets reported. - expect(src.indexOf("'23505'")).toBeLessThan(src.indexOf('route.guidebook.redeem.insert')) + expect(src).toContain(RPC_NAME) + // A plain .insert() here would bypass the increment entirely: the row + // would still be deduped by the index, but open_count would stay 1 forever + // and the engagement number would silently flatline. + expect(src).not.toMatch(/\.from\(\s*['"]guidebook_offer_redemptions['"]\s*\)\s*\.insert/) }) it('the route names the index it depends on, so a grep for the index finds its consumer', () => { diff --git a/unit/inngest/guidebook-sms-morning-cron.test.ts b/unit/inngest/guidebook-sms-morning-cron.test.ts index 14336162..be5e789c 100644 --- a/unit/inngest/guidebook-sms-morning-cron.test.ts +++ b/unit/inngest/guidebook-sms-morning-cron.test.ts @@ -362,4 +362,84 @@ describe('guidebookSmsMorningSend (per-guest handler)', () => { expect(result).toEqual({ optinId: 'optin_1', sent: false }) expect(releaseDailySmsSlot).toHaveBeenCalledWith(supabase, 'optin_1', 'last_morning_sms_date') }) + + // ── Check-in day ────────────────────────────────────────────────────────── + // The cron's eligibility filter is `checkin_date <= today AND checkout_date + // >= today`, so a guest whose stay STARTS today is included — but the cron + // fires 7-11 AM and check-in is typically mid-afternoon. + + it('sends an arrival reminder — not the weather/sponsor nudge — on the guest\'s check-in day', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + properties: [{ data: { ...propertyRow, checkin_time: '16:00:00' }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + const arrivalEvent = { data: { ...sendEvent.data, checkin_date: '2026-07-22' } } // == today_date + + const result = await invokeHandler(guidebookSmsMorningSend, { event: arrivalEvent, step: makeStep() }) + + // Previously this guest got "it's 72°F at your rental, here's a coffee + // spot 0.4 mi away" hours before they had keys. + expect(renderSmsBody).toHaveBeenCalledWith('org_1', 'arrival_reminder', { + property_name: 'Lake House', + checkin_line: 'Just a reminder that check-in is at 4:00 PM.', + }) + expect(renderSmsBody).not.toHaveBeenCalledWith('org_1', 'morning_nudge', expect.anything()) + expect(sendSMS).toHaveBeenCalledWith('+15551234567', 'rendered sms body', { category: 'nudge', orgId: 'org_1' }) + expect(result).toEqual({ optinId: 'optin_1', sent: true }) + }) + + it('does not need weather or coordinates to send the arrival reminder', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + // No lat/lng — a property that could never receive the normal nudge. + properties: [{ data: { id: 'prop_1', name: 'Lake House', lat: null, lng: null, checkin_time: '15:00:00' }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + const arrivalEvent = { data: { ...sendEvent.data, checkin_date: '2026-07-22' } } + const result = await invokeHandler(guidebookSmsMorningSend, { event: arrivalEvent, step: makeStep() }) + + expect(getWeatherForLocation).not.toHaveBeenCalled() + expect(result).toEqual({ optinId: 'optin_1', sent: true }) + }) + + it('omits the check-in sentence entirely when the property has no check-in time', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + // OwnerRez-synced properties explicitly write null here. + properties: [{ data: { ...propertyRow, checkin_time: null }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + const arrivalEvent = { data: { ...sendEvent.data, checkin_date: '2026-07-22' } } + await invokeHandler(guidebookSmsMorningSend, { event: arrivalEvent, step: makeStep() }) + + // Empty, not "check-in is at ." — the template joins on a truthy check. + expect(renderSmsBody).toHaveBeenCalledWith('org_1', 'arrival_reminder', { + property_name: 'Lake House', + checkin_line: '', + }) + }) + + it('still sends the normal nudge mid-stay, including on checkout morning', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + properties: [{ data: propertyRow, error: null }], + guidebook_sponsors: [{ data: [sponsorRow()], error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getWeatherForLocation as ReturnType).mockResolvedValue(clearWeather) + + // checkin_date < today. The send handler is not told checkout_date at all, + // so the outgoing guest on a same-day flip takes exactly this path — they + // ARE still in the house on checkout morning and a local recommendation + // still lands. Deliberately unchanged. + await invokeHandler(guidebookSmsMorningSend, { event: sendEvent, step: makeStep() }) + + expect(renderSmsBody).toHaveBeenCalledWith('org_1', 'morning_nudge', expect.anything()) + expect(renderSmsBody).not.toHaveBeenCalledWith('org_1', 'arrival_reminder', expect.anything()) + }) + }) diff --git a/unit/lib/sms-template-registry.test.ts b/unit/lib/sms-template-registry.test.ts index 6fb728bb..412c8754 100644 --- a/unit/lib/sms-template-registry.test.ts +++ b/unit/lib/sms-template-registry.test.ts @@ -8,6 +8,7 @@ import { const EXPECTED_KEYS: SmsTemplateKey[] = [ 'door_code', 'morning_nudge', + 'arrival_reminder', 'evening_nudge', 'rain_alert', 'stay_extension', @@ -57,7 +58,7 @@ describe('renderTemplate', () => { }) describe('SMS_TEMPLATE_REGISTRY', () => { - it('registers exactly the nine known template keys, each exactly once', () => { + it('registers exactly the ten known template keys, each exactly once', () => { const keys = SMS_TEMPLATE_REGISTRY.map((t) => t.key) expect(keys.sort()).toEqual([...EXPECTED_KEYS].sort()) expect(new Set(keys).size).toBe(keys.length) diff --git a/unit/route-handlers/guidebook-redeem.test.ts b/unit/route-handlers/guidebook-redeem.test.ts index 8dbec3c2..13a3b78d 100644 --- a/unit/route-handlers/guidebook-redeem.test.ts +++ b/unit/route-handlers/guidebook-redeem.test.ts @@ -44,6 +44,11 @@ function makeServiceClient(opts: { bookingResult?: { data: unknown; error?: unknown } insertResult?: { error: unknown } } = {}) { + // The write goes through record_guidebook_offer_open (migration + // 20260807170000), which does the insert-or-increment in one statement — the + // arbiter is a partial EXPRESSION index that PostgREST's on_conflict cannot + // name, and read-then-write in the route would be a TOCTOU. + const rpcMock = vi.fn(() => Promise.resolve(opts.insertResult ?? { error: null })) const insertMock = vi.fn(() => Promise.resolve(opts.insertResult ?? { error: null })) const from = vi.fn((table: string) => { @@ -85,7 +90,7 @@ function makeServiceClient(opts: { return chain }) - return { from, insertMock } + return { from, insertMock, rpc: rpcMock } } function postRequest(body: unknown) { @@ -141,10 +146,11 @@ describe('POST /api/guidebook/redeem', () => { // unattributed, while the guest still saw {ok:true}. The route's own // stated fallback is an anonymous redemption; this is it. expect(res.status).toBe(200) - expect(service.insertMock).toHaveBeenCalledWith({ - org_id: ORG_ID, - sponsor_id: SPONSOR_ID, - booking_id: null, + // p_booking_id omitted, not null: the function declares DEFAULT NULL so + // the anonymous case is expressible without a cast. + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, }) }) @@ -166,7 +172,7 @@ describe('POST /api/guidebook/redeem', () => { expect(res.status).toBe(200) expect(json).toEqual({ ok: true }) - expect(service.insertMock).not.toHaveBeenCalled() + expect(service.rpc).not.toHaveBeenCalled() }) it('returns ok:true without inserting when the sponsor is not active', async () => { @@ -180,7 +186,7 @@ describe('POST /api/guidebook/redeem', () => { expect(res.status).toBe(200) expect(json).toEqual({ ok: true }) - expect(service.insertMock).not.toHaveBeenCalled() + expect(service.rpc).not.toHaveBeenCalled() }) it('logs the redemption without a booking_id when no bookingToken is supplied', async () => { @@ -190,10 +196,11 @@ describe('POST /api/guidebook/redeem', () => { const res = await POST(postRequest({ sponsorId: SPONSOR_ID })) expect(res.status).toBe(200) - expect(service.insertMock).toHaveBeenCalledWith({ - org_id: ORG_ID, - sponsor_id: SPONSOR_ID, - booking_id: null, + // p_booking_id omitted, not null: the function declares DEFAULT NULL so + // the anonymous case is expressible without a cast. + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, }) }) @@ -206,10 +213,10 @@ describe('POST /api/guidebook/redeem', () => { const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) expect(res.status).toBe(200) - expect(service.insertMock).toHaveBeenCalledWith({ - org_id: ORG_ID, - sponsor_id: SPONSOR_ID, - booking_id: BOOKING_ID, + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, + p_booking_id: BOOKING_ID, }) }) @@ -222,37 +229,15 @@ describe('POST /api/guidebook/redeem', () => { const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) expect(res.status).toBe(200) - expect(service.insertMock).toHaveBeenCalledWith({ - org_id: ORG_ID, - sponsor_id: SPONSOR_ID, - booking_id: null, + // p_booking_id omitted, not null: the function declares DEFAULT NULL so + // the anonymous case is expressible without a cast. + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, }) }) - it('treats a 23505 as already-counted-today, not as a failure to report', async () => { - const reportSpy = vi.mocked(reportError) - const service = makeServiceClient({ - insertResult: { - error: { - code: '23505', - message: 'duplicate key value violates unique constraint "uniq_guidebook_offer_redemptions_sponsor_booking_day"', - }, - }, - }) - vi.mocked(createServiceClient).mockReturnValue(service as never) - - const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) - - // Reopening the pass is the normal case — the guest looks at the offer, - // closes it, walks to the business, opens it again for staff. The second - // open must be a no-op, NOT a reported error, or the dedup constraint just - // moves the noise from the sponsor's count into Sentry. - expect(res.status).toBe(200) - expect(await res.json()).toEqual({ ok: true }) - expect(reportSpy).not.toHaveBeenCalled() - }) - - it('still reports a non-23505 insert failure', async () => { + it('reports a write failure from the open-recorder RPC', async () => { const reportSpy = vi.mocked(reportError) const service = makeServiceClient({ insertResult: { error: { code: '42501', message: 'new row violates row-level security policy' } }, @@ -261,6 +246,9 @@ describe('POST /api/guidebook/redeem', () => { const res = await POST(postRequest({ sponsorId: SPONSOR_ID })) + // Best-effort for the guest — they still get their offer — but not silent: + // a sustained failure here is indistinguishable from "nobody redeemed + // anything" in the sponsor's reporting. expect(res.status).toBe(200) expect(reportSpy).toHaveBeenCalledWith( expect.any(Error), From aeb4cc9b7466422a65915673e0c236403b8c9db9 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 17:45:05 +0000 Subject: [PATCH 09/11] fix(guidebook): clear the two semgrep findings and the Sonar assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both semgrep findings were mine, and both were new-vs-base because my previous commit touched those lines. Fixed at the site, not silenced. `guidebook-sms-morning-cron.ts` — every read in the per-guest send now unwraps (4 -> 0 in the error-handling ratchet, entry deleted). The opt-in one is the one that mattered: `{ data: optin }` collapsed "this guest opted out" and "the consent read failed" into the same null, and BOTH ended at `return false` — so a transient failure silently suppressed the guest's message with nothing logged and no retry, while an opt-out is correctly final. The two sponsor reads had the same shape one layer down: a failed lookup produced an empty pool, indistinguishable from an org that simply has no sponsor in that slot. The property read also gains `.eq('org_id', orgId)`. `guidebook-stay-extension-cron.ts` — the per-org bookings read is now paginated via fetchAllRows (removing it from the unbounded-select baseline entirely). One org's checkouts on one exact date is ~properties-per-org, so this could not realistically hit the 1000-row cap — but "realistically" was doing the work in that sentence, and that is precisely the reasoning that left eight platform-wide crons silently truncated until the 2026-07-30 audit. fetchAllRows costs an extra round trip only once the set actually exceeds a page, and removes the assumption rather than restating it. Sonar (unit/inngest/guidebook-sponsor-activated.test.ts L105): the tenant-scope assertion compared two `.filter(...).length` expressions inside expect(), which toHaveLength cannot express. Restructured into named counts, which reads better anyway — and re-canaried: dropping `.eq('org_id', ...)` from the pre-read still fails it. Ratchets updated in the shrink-only direction only: error-handling morning-cron 4 -> 0, unbounded-select entry removed, semgrep read-without-error 157 -> 153 and unbounded-select-org-scoped 98 -> 97. The n+1 EXCEPTIONS line reference moved 87 -> 98 with the code. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- .semgrep/baseline-counts.json | 4 +- .../functions/guidebook-sms-morning-cron.ts | 39 ++++++++++++--- .../guidebook-stay-extension-cron.ts | 49 ++++++++++++------- unit/guardrails/n-plus-one-loops.test.ts | 2 +- .../supabase-error-handling.test.ts | 13 ++++- unit/guardrails/unbounded-select.test.ts | 1 - .../guidebook-sponsor-activated.test.ts | 14 ++++-- 7 files changed, 87 insertions(+), 35 deletions(-) diff --git a/.semgrep/baseline-counts.json b/.semgrep/baseline-counts.json index dd21f6e1..62ad1651 100644 --- a/.semgrep/baseline-counts.json +++ b/.semgrep/baseline-counts.json @@ -100,10 +100,10 @@ "measured_at": "2026-08-07", "counts": { "fieldstay-supabase-discarded-result": 79, - "fieldstay-supabase-read-without-error": 157, + "fieldstay-supabase-read-without-error": 153, "fieldstay-supabase-read-without-error-fan-in": 0, "fieldstay-supabase-unbounded-select-in-list": 34, - "fieldstay-supabase-unbounded-select-org-scoped": 98, + "fieldstay-supabase-unbounded-select-org-scoped": 97, "fieldstay-supabase-unbounded-select-single-parent": 29 } } diff --git a/lib/inngest/functions/guidebook-sms-morning-cron.ts b/lib/inngest/functions/guidebook-sms-morning-cron.ts index d9e81138..6c91bc6f 100644 --- a/lib/inngest/functions/guidebook-sms-morning-cron.ts +++ b/lib/inngest/functions/guidebook-sms-morning-cron.ts @@ -2,6 +2,7 @@ import { asBooleanMap } from '@/lib/json' import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap, unwrapList } from '@/lib/supabase/unwrap' import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' @@ -133,18 +134,33 @@ export const guidebookSmsMorningSend = inngest.createFunction( // Re-fetch instead of trusting the dispatch-time snapshot: is_active // may have flipped (guest texted STOP) since the cron ran. - const { data: optin } = await supabase + // + // Unwrapped, not destructured. `{ data: optin }` collapsed "this guest + // opted out" and "the consent read failed" into the same null, and both + // ended at `return false` — so a transient failure silently suppressed + // the message with nothing logged and no retry. The two outcomes need + // opposite handling: opted-out is final, a failed read must be retried. + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('id, phone_e164, is_active') .eq('id', optinId) .maybeSingle() + + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-sms-morning-send.optin', orgId, + }) if (!optin?.is_active) return false - const { data: property } = await supabase + const propertyRes = await supabase .from('properties') .select('id, name, lat, lng, amenities, checkin_time') .eq('id', propertyId) + .eq('org_id', orgId) .maybeSingle() + + const property = unwrap(propertyRes, { + site: 'inngest.guidebook-sms-morning-send.property', orgId, + }) if (!property) return false // ── Check-in day: this guest has not arrived yet ────────────────────── @@ -202,14 +218,22 @@ export const guidebookSmsMorningSend = inngest.createFunction( // Rain alert takes priority if precip >= 60% and rainy_day sponsor exists if (weather.precipitationProbability >= 60) { - const { data: rainySponsors } = await supabase + // Unwrapped like every other read here: a failed sponsor lookup + // produced an empty pool, which falls through to "no offer" and ends + // at `return false` — a silently skipped nudge that looks identical to + // an org with no rainy-day sponsor. + const rainyRes = await supabase .from('guidebook_sponsors') .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') .eq('org_id', orgId) .eq('status', 'active') .eq('slot_type', 'rainy_day') - const pickedRainy = pickNearestSponsor((rainySponsors ?? []) as GuidebookSponsor[], property.lat, property.lng) + const rainySponsors = unwrapList(rainyRes, { + site: 'inngest.guidebook-sms-morning-send.rainy-sponsors', orgId, + }) + + const pickedRainy = pickNearestSponsor(rainySponsors as GuidebookSponsor[], property.lat, property.lng) if (pickedRainy) { const { sponsor: rainySponsor, distanceMiles: rainyDistanceMi } = pickedRainy @@ -243,14 +267,17 @@ export const guidebookSmsMorningSend = inngest.createFunction( } // Morning brew → general fallback - const { data: sponsorsData } = await supabase + const sponsorsRes = await supabase .from('guidebook_sponsors') .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') .eq('org_id', orgId) .eq('status', 'active') .in('slot_type', ['morning_brew', 'general']) - const orgSponsors = (sponsorsData ?? []) as GuidebookSponsor[] + // Same reasoning as the rainy-day pool above. + const orgSponsors = unwrapList(sponsorsRes, { + site: 'inngest.guidebook-sms-morning-send.sponsors', orgId, + }) as GuidebookSponsor[] const morningBrews = orgSponsors.filter((s) => s.slot_type === 'morning_brew') const pool = morningBrews.length > 0 ? morningBrews : orgSponsors.filter((s) => s.slot_type === 'general') const picked = pickNearestSponsor(pool, property.lat, property.lng) diff --git a/lib/inngest/functions/guidebook-stay-extension-cron.ts b/lib/inngest/functions/guidebook-stay-extension-cron.ts index 34170a2c..dbd13435 100644 --- a/lib/inngest/functions/guidebook-stay-extension-cron.ts +++ b/lib/inngest/functions/guidebook-stay-extension-cron.ts @@ -1,7 +1,7 @@ import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' -import { unwrap, unwrapList } from '@/lib/supabase/unwrap' +import { unwrap } from '@/lib/supabase/unwrap' const FALLBACK_TIMEZONE = 'America/New_York' @@ -63,24 +63,35 @@ export const guidebookStayExtensionCron = inngest.createFunction( Date.now() + config.extension_message_days_before * 24 * 60 * 60 * 1000 ).toISOString().split('T')[0] - // Unwrapped, not destructured. Discarding this error made a failed - // read indistinguishable from "this org has no checkouts that day": - // `bookings` came back null, `?? []` turned it into zero iterations, - // and the cron returned a successful `dispatched: 0`. Bounded by one - // org's checkouts on one exact date, so no pagination is needed — the - // error handling is the whole point here. - const bookingsRes = await supabase - .from('bookings') - .select('id, org_id, property_id, checkout_date') - .eq('org_id', config.org_id) - .eq('checkout_date', targetCheckout) - .eq('status', 'confirmed') - .eq('is_block', false) - - const bookings = unwrapList(bookingsRes, { - site: 'inngest.guidebook-stay-extension-cron.bookings', - orgId: config.org_id, - }) + // Paginated AND error-bound, for two separate reasons. + // + // The error: discarding it made a failed read indistinguishable from + // "this org has no checkouts that day" — `bookings` came back null, + // `?? []` turned it into zero iterations, and the cron returned a + // successful `dispatched: 0`. + // + // The bound: one org's checkouts on one exact date is ~properties-per- + // org (10-50 for the target user), so this cannot realistically reach + // PostgREST's 1000-row cap. "Realistically" is doing the work in that + // sentence, and it is exactly the reasoning that left eight + // platform-wide crons silently truncated until the 2026-07-30 audit. + // fetchAllRows costs one extra round trip only once the set actually + // exceeds a page — i.e. never, on current assumptions — and removes + // the assumption instead of restating it. + const bookings = await fetchAllRows<{ + id: string; org_id: string; property_id: string; checkout_date: string + }>( + (from, to) => supabase + .from('bookings') + .select('id, org_id, property_id, checkout_date') + .eq('org_id', config.org_id) + .eq('checkout_date', targetCheckout) + .eq('status', 'confirmed') + .eq('is_block', false) + .order('id') + .range(from, to), + { label: 'guidebook-stay-extension-cron.bookings' }, + ) let sent = 0 diff --git a/unit/guardrails/n-plus-one-loops.test.ts b/unit/guardrails/n-plus-one-loops.test.ts index f81b53bf..08edeb56 100644 --- a/unit/guardrails/n-plus-one-loops.test.ts +++ b/unit/guardrails/n-plus-one-loops.test.ts @@ -127,7 +127,7 @@ const EXCEPTIONS: Record = { 'Real N+1 (existence-check select + insert per property) left as a known, bounded cost — deferred rather than fixed blind in the same PR that added this guardrail, since it touches live PMS-sync logic. Bounded by properties-per-org (10-50 per CLAUDE.md\'s target user).', 'lib/asset-discovery/seed-from-amenities.ts:171': 'Second pass (absent-asset-types) of the same function — same reasoning as line 63.', - 'lib/inngest/functions/guidebook-stay-extension-cron.ts:87': + 'lib/inngest/functions/guidebook-stay-extension-cron.ts:98': 'Real N+1 (existence check, next-booking lookup, opt-in lookup, insert — 4 queries per booking) left as a known, bounded cost — deferred rather than fixed blind, touches live guest-messaging sync logic. Bounded by same-day checkouts per org per day.', 'lib/inngest/functions/ownerrez/reconciliation-handler.ts:124': 'Real N+1 (cancel booking + cancel its turnovers, per stale booking) left as a known, bounded cost — deferred rather than fixed blind. Contrast lib/inngest/functions/ical-sync.ts, which batches the equivalent booking-cancel via .update().in(\'id\', ids) — a good template for fixing this one later.', diff --git a/unit/guardrails/supabase-error-handling.test.ts b/unit/guardrails/supabase-error-handling.test.ts index dbe8f917..f9b9515c 100644 --- a/unit/guardrails/supabase-error-handling.test.ts +++ b/unit/guardrails/supabase-error-handling.test.ts @@ -130,8 +130,17 @@ const BASELINE: Record = { 'lib/inngest/functions/email-trial-lifecycle.tsx': 4, 'lib/inngest/functions/flagged-turnover-wo.ts': 3, 'lib/inngest/functions/guidebook-sms-evening-cron.ts': 3, - 'lib/inngest/functions/guidebook-sms-morning-cron.ts': 4, - // 5 -> 0 and 4 -> 0 (entries deleted): the gap-night offer's whole failure + // 4 -> 0 (entry deleted): every read in the per-guest send now unwraps. The + // opt-in one mattered most — `{ data: optin }` collapsed "this guest opted + // out" and "the consent read failed" into the same null, and both ended at + // `return false`, so a transient failure silently suppressed the message + // with nothing logged and no retry. The two sponsor reads had the same + // shape: a failed lookup produced an empty pool, indistinguishable from an + // org that simply has no sponsor in that slot. + // + // guidebook-stay-extension-cron.ts 5 -> 0 and + // guidebook-stay-extension-handler.ts 4 -> 0 (both entries deleted): the + // gap-night offer's whole failure // surface was silent. In the cron a failed bookings read looked like "this // org has no checkouts", a failed existence check looked like "not yet // handled", and a failed next-booking read looked like "open calendar" — diff --git a/unit/guardrails/unbounded-select.test.ts b/unit/guardrails/unbounded-select.test.ts index 0f7d6575..34f0293c 100644 --- a/unit/guardrails/unbounded-select.test.ts +++ b/unit/guardrails/unbounded-select.test.ts @@ -130,7 +130,6 @@ const BASELINE = new Set([ 'lib/inngest/functions/flagged-turnover-wo.ts', 'lib/inngest/functions/guidebook-sms-evening-cron.ts', 'lib/inngest/functions/guidebook-sms-morning-cron.ts', - 'lib/inngest/functions/guidebook-stay-extension-cron.ts', 'lib/inngest/functions/hospitable/calendar-sync-handler.ts', 'lib/inngest/functions/hospitable/hospitable-reviews-backfill.ts', 'lib/inngest/functions/hospitable/initial-sync.ts', diff --git a/unit/inngest/guidebook-sponsor-activated.test.ts b/unit/inngest/guidebook-sponsor-activated.test.ts index 45a4ebd9..1cd26faf 100644 --- a/unit/inngest/guidebook-sponsor-activated.test.ts +++ b/unit/inngest/guidebook-sponsor-activated.test.ts @@ -102,10 +102,16 @@ describe('guidebookSponsorActivated', () => { const sponsorEqArgs = supabase.calls .filter((c) => c.table === 'guidebook_sponsors' && c.method === 'eq') .map((c) => JSON.stringify(c.args)) - expect(sponsorEqArgs.filter((a) => a === JSON.stringify(['id', 'sponsor_1'])).length) - .toBe(sponsorEqArgs.filter((a) => a === JSON.stringify(['org_id', 'org_1'])).length) - expect(sponsorEqArgs.length).toBeGreaterThanOrEqual(2) - expect(new Set(sponsorEqArgs)).toEqual( + + const idScoped = sponsorEqArgs.filter((a) => a === JSON.stringify(['id', 'sponsor_1'])).length + const orgScoped = sponsorEqArgs.filter((a) => a === JSON.stringify(['org_id', 'org_1'])).length + const distinctArgs = new Set(sponsorEqArgs) + + // Every id filter is matched by an org filter, so no statement can reach a + // sponsor row on id alone. + expect(orgScoped).toBe(idScoped) + expect(idScoped).toBeGreaterThan(0) + expect(distinctArgs).toEqual( new Set([JSON.stringify(['id', 'sponsor_1']), JSON.stringify(['org_id', 'org_1'])]), ) From fd303f2dd2eeef05aa88a95bce13ecb07c807f85 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 18:08:44 +0000 Subject: [PATCH 10/11] fix(guidebook): bound every sponsor-pool read instead of relocating findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My previous commit renamed those two reads while unwrapping them, which made two LATENT findings new-vs-base under a different pair of rules. I traded one pair of findings for another rather than fixing anything — the durable answer is to actually bound the reads, which is what this does. All three sponsor-pool reads (two morning, one evening) now go through fetchAllRows. guidebook_sponsors is capped at SIX rows per org by the schema itself — slot_number CHECK 1..6 plus UNIQUE(org_id, slot_number) — so this drains in exactly one request and the pagination costs nothing at current scale. A .limit() would have been cheaper to write and would start truncating silently the day that ceiling moves; fetchAllRows throws instead. Also finishes the evening cron's opt-in and property reads, which had the same defect as the morning twin's and which I had left inconsistent: `{ data: optin }` collapsed "this guest opted out" and "the consent read failed" into the same null, and both ended at `return false` — opting out is final, a failed read must retry. Both crons now hold at 0 in the error-handling ratchet, along with both stay-extension files. Two shared-helper extractions fell out of typing this honestly rather than casting past it: - SPONSOR_POOL_COLUMNS + SponsorPoolRow (lib/sms/pick-nearest-sponsor.ts). The reads select a SUBSET of guidebook_sponsors; asserting the result was a full GuidebookSponsor was a cast that happened to compile, not a fact. One column list for all three so the pools cannot drift. - pickNearestSponsor is now generic over anything carrying lat/lng — the only fields it reads — so a narrow SELECT no longer has to cast its way in. - asOfferType (lib/guidebook/offer.ts) narrows offer_type, a TEXT column with a CHECK that PostgREST returns as bare string. Same pattern and reason as asExtensionContactMethod; unrecognised input falls back to 'none', which formatOffer already treats as "no offer" — a sponsor line omitted rather than rendered from a value nothing understands. Ratchets moved shrink-only: read-without-error 153 -> 150, unbounded-select-in-list 34 -> 32, unbounded-select-org-scoped 97 -> 96, both SMS crons deleted from the unbounded-select baseline, evening cron 3 -> 0 in error-handling. Process fix: semgrep is now installed here, so I ran the exact two gates CI runs — `--config .semgrep/ratchet.yml --baseline-commit --error` and `--config .semgrep/chokepoints.yml --error` — both clean. That is what I should have been able to do before the last two pushes; reading the rules and reasoning about which lines moved was not a substitute. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- .semgrep/baseline-counts.json | 6 +- lib/guidebook/offer.ts | 17 +++++ .../functions/guidebook-sms-evening-cron.ts | 53 +++++++++---- .../functions/guidebook-sms-morning-cron.ts | 74 +++++++++++-------- lib/sms/pick-nearest-sponsor.ts | 38 ++++++++-- .../supabase-error-handling.test.ts | 5 +- unit/guardrails/unbounded-select.test.ts | 2 - 7 files changed, 141 insertions(+), 54 deletions(-) diff --git a/.semgrep/baseline-counts.json b/.semgrep/baseline-counts.json index 62ad1651..8430508c 100644 --- a/.semgrep/baseline-counts.json +++ b/.semgrep/baseline-counts.json @@ -100,10 +100,10 @@ "measured_at": "2026-08-07", "counts": { "fieldstay-supabase-discarded-result": 79, - "fieldstay-supabase-read-without-error": 153, + "fieldstay-supabase-read-without-error": 150, "fieldstay-supabase-read-without-error-fan-in": 0, - "fieldstay-supabase-unbounded-select-in-list": 34, - "fieldstay-supabase-unbounded-select-org-scoped": 97, + "fieldstay-supabase-unbounded-select-in-list": 32, + "fieldstay-supabase-unbounded-select-org-scoped": 96, "fieldstay-supabase-unbounded-select-single-parent": 29 } } diff --git a/lib/guidebook/offer.ts b/lib/guidebook/offer.ts index 425a97c1..95b83997 100644 --- a/lib/guidebook/offer.ts +++ b/lib/guidebook/offer.ts @@ -1,5 +1,22 @@ import type { GuidebookOfferType } from '@/types/database' +const OFFER_TYPES: readonly GuidebookOfferType[] = ['percentage', 'fixed_amount', 'item', 'custom', 'none'] + +/** + * Narrows guidebook_sponsors.offer_type — a TEXT column with a CHECK + * constraint, so PostgREST hands it back as a bare `string` — to the union the + * formatters actually branch on. + * + * Same pattern and same reason as asExtensionContactMethod for + * extension_contact_method. Unrecognised input falls back to 'none', which + * formatOffer already treats as "no offer to show" — the safe direction: a + * sponsor line is omitted rather than rendered from a value nothing + * understands. + */ +export function asOfferType(value: string | null | undefined): GuidebookOfferType { + return OFFER_TYPES.includes(value as GuidebookOfferType) ? (value as GuidebookOfferType) : 'none' +} + function formatOfferPrice(value: number): string { return value % 1 === 0 ? String(value) : value.toFixed(2) } diff --git a/lib/inngest/functions/guidebook-sms-evening-cron.ts b/lib/inngest/functions/guidebook-sms-evening-cron.ts index 7760bc0a..e8cf4f0e 100644 --- a/lib/inngest/functions/guidebook-sms-evening-cron.ts +++ b/lib/inngest/functions/guidebook-sms-evening-cron.ts @@ -2,14 +2,15 @@ import { asBooleanMap } from '@/lib/json' import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' import { sendClaimedDailySms } from '@/lib/sms/optin-claim' -import { pickNearestSponsor } from '@/lib/sms/pick-nearest-sponsor' +import { pickNearestSponsor, SPONSOR_POOL_COLUMNS, type SponsorPoolRow } from '@/lib/sms/pick-nearest-sponsor' import { unwrapJoin } from '@/lib/utils/supabase-joins' import { getFeaturedAmenityLine } from '@/lib/guidebook/featured-amenities' -import type { GuidebookSponsor } from '@/types/database' +import { asOfferType } from '@/lib/guidebook/offer' const FALLBACK_TIMEZONE = 'America/New_York' @@ -111,18 +112,32 @@ export const guidebookSmsEveningSend = inngest.createFunction( // Re-fetch instead of trusting the dispatch-time snapshot: is_active // may have flipped (guest texted STOP) since the cron ran. - const { data: optin } = await supabase + // Unwrapped, not destructured — the same pairing as the morning send. + // `{ data: optin }` collapsed "this guest opted out" and "the consent + // read failed" into the same null, and both ended at `return false`, so + // a transient failure silently suppressed the message with nothing + // logged and no retry. Opting out is final; a failed read must retry. + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('id, phone_e164, is_active') .eq('id', optinId) .maybeSingle() + + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-sms-evening-send.optin', orgId, + }) if (!optin?.is_active) return false - const { data: property } = await supabase + const propertyRes = await supabase .from('properties') .select('id, name, lat, lng, amenities') .eq('id', propertyId) + .eq('org_id', orgId) .maybeSingle() + + const property = unwrap(propertyRes, { + site: 'inngest.guidebook-sms-evening-send.property', orgId, + }) if (!property?.lat || !property?.lng) return false const weather = await getWeatherForLocation(property.lat, property.lng).catch(() => null) @@ -139,14 +154,26 @@ export const guidebookSmsEveningSend = inngest.createFunction( rotationOffset: 1, }) - const { data: sponsorsData } = await supabase - .from('guidebook_sponsors') - .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') - .eq('org_id', orgId) - .eq('status', 'active') - .in('slot_type', ['dinner_pints', 'rainy_day', 'general']) - - const orgSponsors = (sponsorsData ?? []) as GuidebookSponsor[] + // Bound and error-handled for the same reasons as the morning send's + // pool: discarding the error made a failed lookup indistinguishable + // from an org with no sponsors, and both end at "no offer" -> no SMS. + // fetchAllRows, not a bare select. guidebook_sponsors is capped at SIX + // rows per org by the schema itself (slot_number CHECK 1..6 plus + // UNIQUE(org_id, slot_number)), so this drains in exactly one request — + // the pagination costs nothing at current scale and stops the read from + // resting on that cap. If the slot ceiling is ever raised, a .limit() + // would have started truncating silently; this throws instead. + const orgSponsors = await fetchAllRows( + (from, to) => supabase + .from('guidebook_sponsors') + .select(SPONSOR_POOL_COLUMNS) + .eq('org_id', orgId) + .eq('status', 'active') + .in('slot_type', ['dinner_pints', 'rainy_day', 'general']) + .order('id') + .range(from, to), + { label: 'guidebook-sms-evening-send.sponsors' }, + ) // Rain → dinner → general fallback const primarySlot = isRainy ? 'rainy_day' : 'dinner_pints' @@ -158,7 +185,7 @@ export const guidebookSmsEveningSend = inngest.createFunction( const sponsorLine = picked ? buildSponsorLine( picked.sponsor.business_name, - picked.sponsor.offer_type, + asOfferType(picked.sponsor.offer_type), picked.sponsor.offer_value, picked.sponsor.offer_item, picked.sponsor.custom_offer_text, diff --git a/lib/inngest/functions/guidebook-sms-morning-cron.ts b/lib/inngest/functions/guidebook-sms-morning-cron.ts index 6c91bc6f..8aa00652 100644 --- a/lib/inngest/functions/guidebook-sms-morning-cron.ts +++ b/lib/inngest/functions/guidebook-sms-morning-cron.ts @@ -2,16 +2,16 @@ import { asBooleanMap } from '@/lib/json' import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' -import { unwrap, unwrapList } from '@/lib/supabase/unwrap' +import { unwrap } from '@/lib/supabase/unwrap' import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' import { sendClaimedDailySms } from '@/lib/sms/optin-claim' import { formatTime12h } from '@/lib/utils/time-of-day' -import { pickNearestSponsor } from '@/lib/sms/pick-nearest-sponsor' +import { pickNearestSponsor, SPONSOR_POOL_COLUMNS, type SponsorPoolRow } from '@/lib/sms/pick-nearest-sponsor' import { unwrapJoin } from '@/lib/utils/supabase-joins' import { getFeaturedAmenityLine } from '@/lib/guidebook/featured-amenities' -import type { GuidebookSponsor } from '@/types/database' +import { asOfferType } from '@/lib/guidebook/offer' const FALLBACK_TIMEZONE = 'America/New_York' @@ -218,22 +218,29 @@ export const guidebookSmsMorningSend = inngest.createFunction( // Rain alert takes priority if precip >= 60% and rainy_day sponsor exists if (weather.precipitationProbability >= 60) { - // Unwrapped like every other read here: a failed sponsor lookup - // produced an empty pool, which falls through to "no offer" and ends - // at `return false` — a silently skipped nudge that looks identical to - // an org with no rainy-day sponsor. - const rainyRes = await supabase - .from('guidebook_sponsors') - .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') - .eq('org_id', orgId) - .eq('status', 'active') - .eq('slot_type', 'rainy_day') - - const rainySponsors = unwrapList(rainyRes, { - site: 'inngest.guidebook-sms-morning-send.rainy-sponsors', orgId, - }) + // A failed sponsor lookup used to produce an empty pool, which falls + // through to "no offer" and ends at `return false` — a silently + // skipped nudge indistinguishable from an org with no rainy-day + // sponsor. fetchAllRows throws instead. + // fetchAllRows, not a bare select. guidebook_sponsors is capped at SIX + // rows per org by the schema itself (slot_number CHECK 1..6 plus + // UNIQUE(org_id, slot_number)), so this drains in exactly one request — + // the pagination costs nothing at current scale and stops the read from + // resting on that cap. If the slot ceiling is ever raised, a .limit() + // would have started truncating silently; this throws instead. + const rainySponsors = await fetchAllRows( + (from, to) => supabase + .from('guidebook_sponsors') + .select(SPONSOR_POOL_COLUMNS) + .eq('org_id', orgId) + .eq('status', 'active') + .eq('slot_type', 'rainy_day') + .order('id') + .range(from, to), + { label: 'guidebook-sms-morning-send.rainy-sponsors' }, + ) - const pickedRainy = pickNearestSponsor(rainySponsors as GuidebookSponsor[], property.lat, property.lng) + const pickedRainy = pickNearestSponsor(rainySponsors, property.lat, property.lng) if (pickedRainy) { const { sponsor: rainySponsor, distanceMiles: rainyDistanceMi } = pickedRainy @@ -249,7 +256,7 @@ export const guidebookSmsMorningSend = inngest.createFunction( async () => { const rainOfferLine = buildSponsorLine( rainySponsor.business_name, - rainySponsor.offer_type, + asOfferType(rainySponsor.offer_type), rainySponsor.offer_value, rainySponsor.offer_item, rainySponsor.custom_offer_text, @@ -267,17 +274,24 @@ export const guidebookSmsMorningSend = inngest.createFunction( } // Morning brew → general fallback - const sponsorsRes = await supabase - .from('guidebook_sponsors') - .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') - .eq('org_id', orgId) - .eq('status', 'active') - .in('slot_type', ['morning_brew', 'general']) - // Same reasoning as the rainy-day pool above. - const orgSponsors = unwrapList(sponsorsRes, { - site: 'inngest.guidebook-sms-morning-send.sponsors', orgId, - }) as GuidebookSponsor[] + // fetchAllRows, not a bare select. guidebook_sponsors is capped at SIX + // rows per org by the schema itself (slot_number CHECK 1..6 plus + // UNIQUE(org_id, slot_number)), so this drains in exactly one request — + // the pagination costs nothing at current scale and stops the read from + // resting on that cap. If the slot ceiling is ever raised, a .limit() + // would have started truncating silently; this throws instead. + const orgSponsors = await fetchAllRows( + (from, to) => supabase + .from('guidebook_sponsors') + .select(SPONSOR_POOL_COLUMNS) + .eq('org_id', orgId) + .eq('status', 'active') + .in('slot_type', ['morning_brew', 'general']) + .order('id') + .range(from, to), + { label: 'guidebook-sms-morning-send.sponsors' }, + ) const morningBrews = orgSponsors.filter((s) => s.slot_type === 'morning_brew') const pool = morningBrews.length > 0 ? morningBrews : orgSponsors.filter((s) => s.slot_type === 'general') const picked = pickNearestSponsor(pool, property.lat, property.lng) @@ -285,7 +299,7 @@ export const guidebookSmsMorningSend = inngest.createFunction( const sponsorLine = picked ? buildSponsorLine( picked.sponsor.business_name, - picked.sponsor.offer_type, + asOfferType(picked.sponsor.offer_type), picked.sponsor.offer_value, picked.sponsor.offer_item, picked.sponsor.custom_offer_text, diff --git a/lib/sms/pick-nearest-sponsor.ts b/lib/sms/pick-nearest-sponsor.ts index 46ffe87a..efef6f52 100644 --- a/lib/sms/pick-nearest-sponsor.ts +++ b/lib/sms/pick-nearest-sponsor.ts @@ -1,5 +1,29 @@ import { distanceMiles } from '@/lib/geocoding' -import type { GuidebookSponsor } from '@/types/database' + +/** + * The columns the SMS nudge crons actually read off a sponsor, and the SELECT + * string that fetches exactly them. + * + * Shared so the morning and evening pools cannot drift apart, and so the reads + * can be typed honestly: they select a SUBSET of guidebook_sponsors, and + * asserting the result is a full GuidebookSponsor was a cast that happened to + * compile rather than a fact. + */ +export const SPONSOR_POOL_COLUMNS = + 'id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type' + +export interface SponsorPoolRow { + id: string + org_id: string + business_name: string + offer_type: string + offer_value: number | null + offer_item: string | null + custom_offer_text: string | null + lat: number | null + lng: number | null + slot_type: string +} /** * Picks the sponsor nearest to the given property coordinates. Sponsors @@ -7,18 +31,22 @@ import type { GuidebookSponsor } from '@/types/database' * one is used as the fallback). Previously duplicated verbatim in both the * morning and evening SMS nudge crons. */ -export function pickNearestSponsor( - sponsors: GuidebookSponsor[], +// Generic over anything carrying coordinates: the only fields this function +// reads are lat/lng, so constraining callers to a full GuidebookSponsor forced +// every narrow SELECT to cast its way in. The caller keeps its own row type and +// gets it back on `.sponsor`. +export function pickNearestSponsor( + sponsors: T[], lat: number, lng: number -): { sponsor: GuidebookSponsor; distanceMiles: number | null } | null { +): { sponsor: T; distanceMiles: number | null } | null { const withCoords = sponsors.filter((s) => s.lat !== null && s.lng !== null) if (withCoords.length === 0) { const fallback = sponsors[0] return fallback ? { sponsor: fallback, distanceMiles: null } : null } - let nearest: GuidebookSponsor | null = null + let nearest: T | null = null let nearestDist = Infinity for (const s of withCoords) { const dist = distanceMiles(lat, lng, s.lat!, s.lng!) diff --git a/unit/guardrails/supabase-error-handling.test.ts b/unit/guardrails/supabase-error-handling.test.ts index f9b9515c..e69a629d 100644 --- a/unit/guardrails/supabase-error-handling.test.ts +++ b/unit/guardrails/supabase-error-handling.test.ts @@ -129,7 +129,10 @@ const BASELINE: Record = { 'lib/inngest/functions/cron/work-order-ops.ts': 2, 'lib/inngest/functions/email-trial-lifecycle.tsx': 4, 'lib/inngest/functions/flagged-turnover-wo.ts': 3, - 'lib/inngest/functions/guidebook-sms-evening-cron.ts': 3, + // 3 -> 0 (entry deleted), same three reads as its morning twin below: a + // failed sponsor lookup was indistinguishable from an org with no sponsors, + // and a failed opt-in read from a guest who opted out — every one of them + // ending at "no SMS" with nothing logged. // 4 -> 0 (entry deleted): every read in the per-guest send now unwraps. The // opt-in one mattered most — `{ data: optin }` collapsed "this guest opted // out" and "the consent read failed" into the same null, and both ended at diff --git a/unit/guardrails/unbounded-select.test.ts b/unit/guardrails/unbounded-select.test.ts index 34f0293c..cec7a1a7 100644 --- a/unit/guardrails/unbounded-select.test.ts +++ b/unit/guardrails/unbounded-select.test.ts @@ -128,8 +128,6 @@ const BASELINE = new Set([ 'lib/inngest/functions/crew-assignment.ts', 'lib/inngest/functions/cron/daily-wrapup.ts', 'lib/inngest/functions/flagged-turnover-wo.ts', - 'lib/inngest/functions/guidebook-sms-evening-cron.ts', - 'lib/inngest/functions/guidebook-sms-morning-cron.ts', 'lib/inngest/functions/hospitable/calendar-sync-handler.ts', 'lib/inngest/functions/hospitable/hospitable-reviews-backfill.ts', 'lib/inngest/functions/hospitable/initial-sync.ts', From f151ed3ec60f69d3ed40e81af4ee59e776dac628 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 18:22:10 +0000 Subject: [PATCH 11/11] fix(guidebook): make the offer-type list exhaustive, not just a membership check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Sonar finding (use a Set, not an array, for existence checks) is not worth acting on by itself — `.includes()` over five frozen strings is if anything faster than a Set, and this runs once per SMS. But it pointed at a spot with a real weakness, which I introduced two commits ago. The array had no compile-time link back to GuidebookOfferType. Add a value to the union and the array silently would not contain it, so asOfferType() would downgrade it to 'none' and every sponsor using that offer type would render no offer at all — with nothing failing anywhere. That is the nasty shape of it: the fallback that makes UNKNOWN input safe is exactly what makes a MISSING entry invisible. A forgotten member looks identical to a genuinely unrecognised value, so no runtime assertion can tell them apart. The list has to be checked by the compiler rather than by whoever remembers to update it. Now a Record, which TypeScript checks for exhaustiveness. Verified by adding 'bogo' to the union: tsc fails at offer.ts naming the missing property. With the previous array it stayed green. The lookup Set falls out of that (Object.keys), which incidentally satisfies Sonar — but the Record is the point, not the Set. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR --- lib/guidebook/offer.ts | 26 +++++++++++++++++++++-- unit/lib/guidebook-offer.test.ts | 36 +++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/lib/guidebook/offer.ts b/lib/guidebook/offer.ts index 95b83997..d662ce75 100644 --- a/lib/guidebook/offer.ts +++ b/lib/guidebook/offer.ts @@ -1,6 +1,28 @@ import type { GuidebookOfferType } from '@/types/database' -const OFFER_TYPES: readonly GuidebookOfferType[] = ['percentage', 'fixed_amount', 'item', 'custom', 'none'] +/** + * Every member of GuidebookOfferType, as a Record so TypeScript enforces + * EXHAUSTIVENESS: add a value to the union and this stops compiling until it + * is listed here. + * + * That, not lookup speed, is the reason this is not a plain array. A + * five-element array's `.includes()` is if anything faster than a Set — but an + * array has no compile-time link back to the union, so a newly added offer + * type would silently fall through asOfferType() to 'none' and every sponsor + * using it would render no offer at all, with nothing failing anywhere. The + * fallback that makes unknown input safe is exactly what makes a MISSING entry + * invisible, so the list has to be checked by the compiler rather than by + * whoever remembers to update it. + */ +const OFFER_TYPES: Record = { + percentage: true, + fixed_amount: true, + item: true, + custom: true, + none: true, +} + +const OFFER_TYPE_KEYS = new Set(Object.keys(OFFER_TYPES)) /** * Narrows guidebook_sponsors.offer_type — a TEXT column with a CHECK @@ -14,7 +36,7 @@ const OFFER_TYPES: readonly GuidebookOfferType[] = ['percentage', 'fixed_amount' * understands. */ export function asOfferType(value: string | null | undefined): GuidebookOfferType { - return OFFER_TYPES.includes(value as GuidebookOfferType) ? (value as GuidebookOfferType) : 'none' + return OFFER_TYPE_KEYS.has(value ?? '') ? (value as GuidebookOfferType) : 'none' } function formatOfferPrice(value: number): string { diff --git a/unit/lib/guidebook-offer.test.ts b/unit/lib/guidebook-offer.test.ts index fe71d0de..cdfbb2b9 100644 --- a/unit/lib/guidebook-offer.test.ts +++ b/unit/lib/guidebook-offer.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { formatOffer } from '@/lib/guidebook/offer' +import { formatOffer, asOfferType } from '@/lib/guidebook/offer' describe('formatOffer', () => { describe('percentage', () => { @@ -76,3 +76,37 @@ describe('formatOffer', () => { }) }) }) + +describe('asOfferType', () => { + // guidebook_sponsors.offer_type is a TEXT column with a CHECK constraint, so + // PostgREST hands it back as a bare string. This is the narrowing boundary. + + it.each(['percentage', 'fixed_amount', 'item', 'custom', 'none'])( + 'passes through the known offer type %s', + (value) => { + expect(asOfferType(value)).toBe(value) + }, + ) + + it.each([ + ['an unknown string', 'bogo'], + ['an empty string', ''], + ['null', null], + ['undefined', undefined], + ])('falls back to none for %s', (_label, value) => { + // 'none' is the safe direction: formatOffer renders nothing for it, so a + // sponsor line is omitted rather than built from a value nothing + // understands. + expect(asOfferType(value)).toBe('none') + }) + + it('the fallback that makes unknown input safe is what makes a MISSING entry invisible', () => { + // Which is why the member list is a Record and + // not an array: adding a value to the union fails the BUILD until it is + // listed. There is no runtime assertion that can catch this — a forgotten + // entry looks exactly like a genuinely unknown value, and every sponsor + // using it would silently show no offer. Verified by reverting the Record + // to an array and adding a union member: tsc stayed green. + expect(asOfferType('bogo')).toBe('none') + }) +})