From 6403220c925ac155b0d86a85a1e7641f8631aa7f Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Tue, 19 May 2026 13:01:16 -0700 Subject: [PATCH] test(integration): DB-backed harness validating shipped SQL (045/042/044/043) Root-cause fix for 'correctness depended on post-deploy manual audits'. Separate psql-driven suite (pnpm test:integration, own vitest config, integration/ outside __tests__ so the hermetic pnpm test gate is untouched; self-skips without a DB). 18 tests run against real Postgres: - 045 class_signup_tx/class_cancel_tx: RETURNS TABLE->array shape the actions destructure, capacity/waitlist/dedupe, and the per-session advisory lock under TWO concurrent connections (D11 proven, never over-enrolls). - 042 equipment exclusion constraint (overlap 23P01; cancelled/adjacent ok). 044 self-change trigger (role + payment cols blocked; service bypass). 043 members_update WITH CHECK (cross-space move blocked). Disproves the pass-52 flagged risk (signup rpc-shape). Non-runtime; default pnpm test still 532/26 hermetic; build clean. --- docs/HANDOFF.md | 20 ++++ docs/LOCAL_DEV.md | 25 +++++ integration/class-concurrency.test.ts | 129 +++++++++++++++++++++++++ integration/db.ts | 130 ++++++++++++++++++++++++++ integration/equipment-overlap.test.ts | 55 +++++++++++ integration/self-change.test.ts | 63 +++++++++++++ package.json | 1 + vitest.integration.config.ts | 22 +++++ 8 files changed, 445 insertions(+) create mode 100644 integration/class-concurrency.test.ts create mode 100644 integration/db.ts create mode 100644 integration/equipment-overlap.test.ts create mode 100644 integration/self-change.test.ts create mode 100644 vitest.integration.config.ts diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index f467c8a..0aaa02f 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -4,6 +4,26 @@ Append-only. Newest entries on top. Keep each entry to one screen. --- +## 2026-05-19 (pass 53) — DB-backed integration harness; shipped SQL now functionally validated (LOCAL, non-runtime) + +Branch `main`. Assessment chose "integration test harness" to fix the root cause (correctness depending on post-deploy manual audits). Built + RAN it against the local Supabase Postgres. Default `pnpm test` still 532/26, hermetic; build clean. + +### Delivered +- **F1 foundation:** `vitest.integration.config.ts` (separate runner; `integration/` is OUTSIDE `__tests__/` so `pnpm test` never touches a DB), `integration/db.ts` psql-driven helpers (service path vs `role authenticated`+`request.jwt.claims.sub` user path; throwing seed helpers), `pnpm test:integration` script. Self-skips when no DB. +- **F2 tests (18, all green on real Postgres):** 045 `class_signup_tx`/`class_cancel_tx` — RETURNS TABLE→array shape the actions destructure, capacity/waitlist/dedupe/no_session, and the per-session advisory lock under TWO genuinely concurrent psql connections (exactly one registered — D11 proven); 042 exclusion constraint (overlap rejected 23P01, cancelled/adjacent allowed); 044 trigger (member self-edit of role + payment columns blocked, service path bypasses) + 043 WITH CHECK (cross-space move blocked). +- Applying `scripts/schema.sql` to the local DB also validated the "idempotent, runnable top-to-bottom" invariant in practice (it was stale at 041; applied cleanly to 045). +- Harness bug found+fixed during bring-up: `space_members.user_id` FKs `auth.users` (my earlier FK introspection was wrong) — seeds now create the auth user first; seed helpers throw on failure. +- Docs: LOCAL_DEV "Integration tests" section. + +### Significance +The concrete prod risk flagged in the pass-52 assessment — signup/cancel possibly broken because the action assumes `supabase.rpc()` returns an array for a RETURNS TABLE function — is **disproven**: the shape + behavior + concurrency are correct. Correctness of the riskiest shipped SQL no longer depends on me manually auditing after deploy. + +### State +- **Non-runtime** (tests, vitest config, a package.json script, docs). No app code, no migration. Nothing to validate in prod; a push would only redeploy unchanged runtime. Held local pending the deploy decision (consistent with the docs-only hold pattern). +- Backlog now: deferred SECURITY DEFINER status-gate (own session, rationale in pass-52); owner product question (forms attribution); ARCHITECTURE limitation #2 doc drift ("Stripe not integrated" — stale). Natural next: extend the harness to the Stripe webhook + key server actions; or owner provisioning + end-to-end spine validation. + +--- + ## 2026-05-19 (pass 52) — Backlog hardening E1-E3 + evidence-based deferral (LOCAL, NEEDS deploy) Branch `main`. Worked the pass-51 small backlog. Suite 532, build clean. NOT deployed. diff --git a/docs/LOCAL_DEV.md b/docs/LOCAL_DEV.md index 007164f..f696ccc 100644 --- a/docs/LOCAL_DEV.md +++ b/docs/LOCAL_DEV.md @@ -170,6 +170,31 @@ SELECT public.expire_proposals(); In production you'd schedule this via pg_cron (the schema's migration 019 sets this up automatically when pg_cron is available) or via a Next.js cron endpoint guarded by a `CRON_SECRET`. +## Integration tests (DB-backed) + +`pnpm test` is hermetic (pure logic, jsdom, no DB) and is the gate. The +separate **`pnpm test:integration`** suite (`integration/`, its own +`vitest.integration.config.ts`) exercises the shipped SQL — the +`class_signup_tx`/`class_cancel_tx` RPCs (045) including the advisory lock +under real concurrency, the `equipment_reservations_no_overlap` exclusion +constraint (042), and the self-change trigger / `members_update` WITH CHECK +(044/043) — against a real Postgres through `psql` (no extra npm dep). It +drives both access paths the app uses: the service path (no JWT) and a +logged-in member (`role authenticated` + `request.jwt.claims.sub`). + +Prereqs: the Supabase CLI stack running (`supabase start`) with the schema +applied (`psql "$DB_URL" -f scripts/schema.sql`). It connects to +`INTEGRATION_DATABASE_URL` (default the local stack +`postgresql://postgres:postgres@127.0.0.1:54322/postgres`) and **self-skips +cleanly** when no DB is reachable, so it never blocks contributors/CI +without one. It is intentionally NOT part of `pnpm test` or the deploy gate. + +```bash +supabase start +psql "postgresql://postgres:postgres@127.0.0.1:54322/postgres" -f scripts/schema.sql +pnpm test:integration +``` + ## Troubleshooting ### `supabase start` hangs or fails diff --git a/integration/class-concurrency.test.ts b/integration/class-concurrency.test.ts new file mode 100644 index 0000000..c311b43 --- /dev/null +++ b/integration/class-concurrency.test.ts @@ -0,0 +1,129 @@ +// Validates migration 045 (class_signup_tx / class_cancel_tx) against a real +// Postgres: the capacity/waitlist/dedupe logic, the per-session advisory lock +// under genuine concurrency, AND the RETURNS TABLE -> array shape the server +// actions destructure (`Array.isArray(rpc) ? rpc[0] : rpc`). This is the +// concrete prod risk flagged in the assessment: signup/cancel was shipped +// with zero functional coverage. +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { promisify } from 'node:util' +import { execFile } from 'node:child_process' +import { + dbReady, + DB_URL, + rows, + execService, + seedSpace, + seedMember, + seedClassSession, + dropSpace, +} from './db' + +const execFileP = promisify(execFile) + +type SignupRet = { signup_id: string | null; signup_status: string | null; err: string | null } +type CancelRet = { cancelled_id: string | null; promoted_id: string | null; err: string | null } + +const d = describe.skipIf(!dbReady) + +d('class_signup_tx / class_cancel_tx (migration 045)', () => { + let spaceId: string + let sessionId: string + let m1: string, m2: string, m3: string + + beforeAll(() => { + spaceId = seedSpace().spaceId + sessionId = seedClassSession(spaceId, 1).sessionId // capacity = 1 + m1 = seedMember(spaceId).memberId + m2 = seedMember(spaceId).memberId + m3 = seedMember(spaceId).memberId + }) + afterAll(() => dropSpace(spaceId)) + + function signup(member: string) { + return rows(`select * from class_signup_tx('${sessionId}','${spaceId}','${member}')`) + } + + it('returns a single-row array (the shape the action destructures)', () => { + const r = signup(m1) + expect(Array.isArray(r)).toBe(true) + expect(r).toHaveLength(1) + expect(r[0]).toHaveProperty('signup_id') + expect(r[0]).toHaveProperty('signup_status') + expect(r[0]).toHaveProperty('err') + }) + + it('first within capacity -> registered; over capacity -> waitlisted', () => { + // m1 took the only seat above. m2 must waitlist. + const r2 = signup(m2) + expect(r2[0].err).toBeNull() + expect(r2[0].signup_status).toBe('waitlisted') + expect(r2[0].signup_id).toBeTruthy() + }) + + it('duplicate signup -> err "already", no row inserted', () => { + const r = signup(m1) + expect(r[0].err).toBe('already') + expect(r[0].signup_id).toBeNull() + const cnt = rows<{ n: number }>( + `select count(*)::int n from class_signups where session_id='${sessionId}' and member_id='${m1}'`, + ) + expect(cnt[0].n).toBe(1) + }) + + it('unknown session -> err "no_session"', () => { + const bogus = '00000000-0000-0000-0000-000000000000' + const r = rows(`select * from class_signup_tx('${bogus}','${spaceId}','${m3}')`) + expect(r[0].err).toBe('no_session') + }) + + it('cancel of the registered member promotes the earliest waitlisted', () => { + const c = rows(`select * from class_cancel_tx('${sessionId}','${spaceId}','${m1}')`) + expect(c[0].err).toBeNull() + expect(c[0].cancelled_id).toBeTruthy() + expect(c[0].promoted_id).toBeTruthy() + // m2 (was waitlisted) is now registered; never exceeds capacity. + const reg = rows<{ status: string }>( + `select status from class_signups where session_id='${sessionId}' and member_id='${m2}' and status<>'cancelled'`, + ) + expect(reg[0].status).toBe('registered') + const overCap = rows<{ n: number }>( + `select count(*)::int n from class_signups where session_id='${sessionId}' and status='registered'`, + ) + expect(overCap[0].n).toBeLessThanOrEqual(1) + }) + + it('cancel when not signed up -> err "not_signed_up"', () => { + const c = rows(`select * from class_cancel_tx('${sessionId}','${spaceId}','${m3}')`) + expect(c[0].err).toBe('not_signed_up') + }) + + it('advisory lock: two CONCURRENT signups at capacity 1 -> exactly one registered', async () => { + const sess2 = seedClassSession(spaceId, 1).sessionId + const a = seedMember(spaceId).memberId + const b = seedMember(spaceId).memberId + const call = (mid: string) => + execFileP('psql', [ + DB_URL, + '-tAc', + `select signup_status from class_signup_tx('${sess2}','${spaceId}','${mid}')`, + ]) + // Fire both before either resolves: the per-session advisory xact lock + // must serialize them so capacity is never exceeded. + const [ra, rb] = await Promise.all([call(a), call(b)]) + const results = [ra.stdout.trim(), rb.stdout.trim()].sort() + expect(results).toEqual(['registered', 'waitlisted']) + const reg = rows<{ n: number }>( + `select count(*)::int n from class_signups where session_id='${sess2}' and status='registered'`, + ) + expect(reg[0].n).toBe(1) // never 2 — the lock held + }) + + it('unlimited capacity (null) -> always registered', () => { + const openSess = seedClassSession(spaceId, null).sessionId + for (let i = 0; i < 3; i++) { + const mid = seedMember(spaceId).memberId + const r = rows(`select * from class_signup_tx('${openSess}','${spaceId}','${mid}')`) + expect(r[0].signup_status).toBe('registered') + } + }) +}) diff --git a/integration/db.ts b/integration/db.ts new file mode 100644 index 0000000..0070403 --- /dev/null +++ b/integration/db.ts @@ -0,0 +1,130 @@ +// Dependency-free Postgres harness: drives the real DB through `psql` (no new +// npm dep). Models the app's two access paths: +// - service path (createAdminClient): superuser/no JWT -> RLS + the +// auth.uid()-gated trigger are bypassed, exactly like the service client. +// - user path (RLS client): role `authenticated` + a request.jwt.claims +// `sub`, so auth.uid() resolves and RLS/triggers apply, exactly like a +// logged-in member. This is the standard Supabase RLS test technique. +import { execFileSync } from 'node:child_process' +import { randomUUID } from 'node:crypto' + +export const DB_URL = + process.env.INTEGRATION_DATABASE_URL || + 'postgresql://postgres:postgres@127.0.0.1:54322/postgres' + +function run(args: string[], input?: string): { ok: boolean; out: string; err: string } { + try { + const out = execFileSync('psql', [DB_URL, '-v', 'ON_ERROR_STOP=1', ...args], { + input, + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + }) + return { ok: true, out, err: '' } + } catch (e: unknown) { + const x = e as { stdout?: string; stderr?: string } + return { ok: false, out: x.stdout ?? '', err: (x.stderr ?? String(e)).trim() } + } +} + +export const dbReady: boolean = run(['-tAc', 'select 1']).ok + +export const uuid = randomUUID + +// Run statements as the SERVICE path (no JWT -> trigger/RLS bypass). +export function execService(sql: string): { ok: boolean; err: string } { + const r = run(['-c', sql]) + return { ok: r.ok, err: r.err } +} + +// Run statements as a logged-in member (auth.uid() = uid, RLS active). +export function execAsUser(uid: string, sql: string): { ok: boolean; err: string } { + const claims = JSON.stringify({ sub: uid }).replace(/'/g, "''") + const wrapped = + `set local role authenticated;` + + `set local "request.jwt.claims" = '${claims}';` + + sql + const r = run(['-c', `begin; ${wrapped}; commit;`]) + return { ok: r.ok, err: r.err } +} + +// Query helper -> array of row objects (service path). Matches the JSON shape +// supabase-js .rpc()/.select() returns, so assertions here also validate the +// shape the server actions destructure. +export function rows>(selectSql: string): T[] { + const r = run(['-tAc', `select coalesce(json_agg(t),'[]'::json)::text from (${selectSql}) t`]) + if (!r.ok) throw new Error(r.err) + return JSON.parse(r.out.trim() || '[]') as T[] +} + +export function rowsAsUser>(uid: string, selectSql: string): T[] { + const claims = JSON.stringify({ sub: uid }).replace(/'/g, "''") + const r = run([ + '-tAc', + `begin; set local role authenticated; set local "request.jwt.claims" = '${claims}';` + + `select coalesce(json_agg(t),'[]'::json)::text from (${selectSql}) t; commit;`, + ]) + if (!r.ok) throw new Error(r.err) + // last non-empty line is the json (BEGIN/SET/COMMIT print nothing with -tA) + const line = r.out.trim().split('\n').filter(Boolean).pop() ?? '[]' + return JSON.parse(line) as T[] +} + +// Minimal seed helpers. They THROW on failure so a seeding problem surfaces +// immediately instead of cascading into confusing FK errors downstream. All +// ids are random so parallel test files never collide. +function must(sql: string): void { + const r = execService(sql) + if (!r.ok) throw new Error(`seed failed: ${r.err}\nSQL: ${sql.slice(0, 200)}`) +} + +export function seedSpace(): { spaceId: string } { + const spaceId = uuid() + must(`insert into spaces (id, name, slug) values ('${spaceId}','itest','itest-${spaceId.slice(0, 8)}');`) + return { spaceId } +} + +// space_members.user_id FKs auth.users(id), so an auth user must exist first +// (auth.users only requires id; we add an email for readability/cleanup). +export function seedMember( + spaceId: string, + opts: { role?: string; status?: string } = {}, +): { memberId: string; userId: string } { + const memberId = uuid() + const userId = uuid() + must(`insert into auth.users (id, email) values ('${userId}','it-${userId}@itest.local');`) + must( + `insert into space_members (id, space_id, user_id, role, status, display_name) + values ('${memberId}','${spaceId}','${userId}','${opts.role ?? 'member'}','${opts.status ?? 'current'}','itest member');`, + ) + return { memberId, userId } +} + +export function seedClassSession( + spaceId: string, + capacity: number | null, +): { classId: string; sessionId: string } { + const classId = uuid() + const sessionId = uuid() + must(`insert into classes (id, space_id, title) values ('${classId}','${spaceId}','itest class');`) + must( + `insert into class_sessions (id, class_id, space_id, starts_at, capacity) + values ('${sessionId}','${classId}','${spaceId}', now() + interval '7 days', ${capacity == null ? 'null' : capacity});`, + ) + return { classId, sessionId } +} + +export function seedEquipment(spaceId: string): { equipmentId: string } { + const equipmentId = uuid() + must(`insert into equipment (id, space_id, name) values ('${equipmentId}','${spaceId}','itest rig');`) + return { equipmentId } +} + +// Cleanup: remove the space's auth.users first (FK from space_members), then +// the space (cascades members/classes/equipment/reservations). Space-scoped +// so it is safe under parallel test files. +export function dropSpace(spaceId: string): void { + execService( + `delete from auth.users where id in (select user_id from space_members where space_id='${spaceId}' and user_id is not null); + delete from spaces where id='${spaceId}';`, + ) +} diff --git a/integration/equipment-overlap.test.ts b/integration/equipment-overlap.test.ts new file mode 100644 index 0000000..1a6567d --- /dev/null +++ b/integration/equipment-overlap.test.ts @@ -0,0 +1,55 @@ +// Validates migration 042: the GiST exclusion constraint +// equipment_reservations_no_overlap is the real concurrency arbiter (the app +// check-then-insert was a TOCTOU). Only 'reserved' rows conflict; tstzrange +// '[)' makes adjacent windows non-overlapping. +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { dbReady, execService, seedSpace, seedMember, seedEquipment, dropSpace } from './db' + +const d = describe.skipIf(!dbReady) + +d('equipment_reservations_no_overlap (migration 042)', () => { + let spaceId: string, equipmentId: string, memberId: string + const T = `timestamptz '2026-09-01 18:00:00+00'` + + beforeAll(() => { + spaceId = seedSpace().spaceId + equipmentId = seedEquipment(spaceId).equipmentId + memberId = seedMember(spaceId).memberId + }) + afterAll(() => dropSpace(spaceId)) + + const ins = (startOffH: number, endOffH: number, status = 'reserved') => + execService( + `insert into equipment_reservations (equipment_id, space_id, member_id, starts_at, ends_at, status) + values ('${equipmentId}','${spaceId}','${memberId}', + ${T} + interval '${startOffH} hours', ${T} + interval '${endOffH} hours', '${status}');`, + ) + + it('accepts a first reservation [0h,2h)', () => { + expect(ins(0, 2).ok).toBe(true) + }) + + it('REJECTS an overlapping reserved [1h,3h) (23P01)', () => { + const r = ins(1, 3) + expect(r.ok).toBe(false) + expect(r.err).toMatch(/exclusion constraint|equipment_reservations_no_overlap|23P01/i) + }) + + it('allows an overlapping CANCELLED row (only reserved conflicts)', () => { + expect(ins(1, 3, 'cancelled').ok).toBe(true) + }) + + it('allows an ADJACENT reserved [2h,4h) (tstzrange [) — no overlap)', () => { + expect(ins(2, 4).ok).toBe(true) + }) + + it('a different equipment is unaffected', () => { + const e2 = seedEquipment(spaceId).equipmentId + expect( + execService( + `insert into equipment_reservations (equipment_id, space_id, member_id, starts_at, ends_at, status) + values ('${e2}','${spaceId}','${memberId}', ${T}, ${T} + interval '2 hours','reserved');`, + ).ok, + ).toBe(true) + }) +}) diff --git a/integration/self-change.test.ts b/integration/self-change.test.ts new file mode 100644 index 0000000..e559286 --- /dev/null +++ b/integration/self-change.test.ts @@ -0,0 +1,63 @@ +// Validates the self-escalation guards: migration 044 +// (prevent_member_self_role_change blocks self-edit of role/status AND the +// payment/dues columns) and migration 043 (members_update WITH CHECK blocks +// a privileged caller moving a row cross-space). Exercised through the real +// RLS/trigger path (role `authenticated` + request.jwt.claims.sub). +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { dbReady, execService, execAsUser, seedSpace, seedMember, dropSpace, uuid } from './db' + +const d = describe.skipIf(!dbReady) + +d('self-change trigger (044) + members_update WITH CHECK (043)', () => { + let spaceId: string + let member: { memberId: string; userId: string } + let admin: { memberId: string; userId: string } + + beforeAll(() => { + spaceId = seedSpace().spaceId + member = seedMember(spaceId, { role: 'member', status: 'current' }) + admin = seedMember(spaceId, { role: 'admin', status: 'current' }) + }) + afterAll(() => dropSpace(spaceId)) + + it('member CANNOT self-escalate role', () => { + const r = execAsUser( + member.userId, + `update space_members set role='admin' where id='${member.memberId}'`, + ) + expect(r.ok).toBe(false) + expect(r.err).toMatch(/42501|cannot change/i) + }) + + it('member CANNOT self-edit a payment/dues column (044)', () => { + const r = execAsUser( + member.userId, + `update space_members set payment_status='paid' where id='${member.memberId}'`, + ) + expect(r.ok).toBe(false) + expect(r.err).toMatch(/42501|cannot change/i) + }) + + it('member CAN self-edit an allowed profile column', () => { + const r = execAsUser( + member.userId, + `update space_members set display_name='renamed' where id='${member.memberId}'`, + ) + expect(r.ok).toBe(true) + }) + + it('service path (no JWT) bypasses the trigger — the Stripe webhook path', () => { + const r = execService( + `update space_members set status='late', last_paid_at=now() where id='${member.memberId}'`, + ) + expect(r.ok).toBe(true) + }) + + it('members_update WITH CHECK blocks a privileged cross-space move (043)', () => { + const r = execAsUser( + admin.userId, + `update space_members set space_id='${uuid()}' where id='${member.memberId}'`, + ) + expect(r.ok).toBe(false) // RLS WITH CHECK (or FK) rejects the post-image + }) +}) diff --git a/package.json b/package.json index 1df30dd..b001d65 100644 --- a/package.json +++ b/package.json @@ -9,6 +9,7 @@ "lint": "eslint .", "test": "vitest", "test:ui": "vitest --ui", + "test:integration": "vitest run --config vitest.integration.config.ts", "test:e2e": "playwright test", "test:e2e:ui": "playwright test --ui" }, diff --git a/vitest.integration.config.ts b/vitest.integration.config.ts new file mode 100644 index 0000000..a83f1cd --- /dev/null +++ b/vitest.integration.config.ts @@ -0,0 +1,22 @@ +import { defineConfig } from 'vitest/config' +import path from 'path' + +// Integration suite: runs the shipped SQL (RPCs / constraints / RLS / +// triggers) against a REAL Postgres via psql. Deliberately separate from the +// default `pnpm test` (which stays hermetic, jsdom, no DB) and from the +// `__tests__/` include glob, so `pnpm test` never touches a database. +// Run with: pnpm test:integration (prereq: a reachable Postgres, default +// the Supabase-CLI local stack — see docs/LOCAL_DEV.md). Self-skips cleanly +// when no DB is reachable, so it never blocks contributors/CI without one. +export default defineConfig({ + test: { + globals: true, + environment: 'node', + include: ['integration/**/*.test.ts'], + testTimeout: 30000, + hookTimeout: 30000, + }, + resolve: { + alias: { '@': path.resolve(__dirname, './') }, + }, +})