Stage each paid order before payment, and harden the whole flow

AGENTS.md

@@ -73,6 +73,7 @@ as-is and skips the download, so `deno task test`, `deno task test:files`, and
 - **Malleable software**: Prefer being up front with operators about the underlying data structure over hiding it. Where it's safe, expose stored records directly and give the operator a page to view and edit them — including aggregated/derived numbers — rather than treating the DB as a black box. The per-contact record editor at `/admin/history/:hmac` (raw booking/message counts plus the private note, keyed by the contact's HMAC) is the reference example. Repairing data should be a first-class operator action, not a manual DB surgery.
 - **Never render a dead or forbidden link**: Don't emit a link the viewer can't follow — one whose target would 404, or whose page the current user's admin level can't open. A rendered link is a promise that it works, so gate it on the same condition the target enforces; when that condition fails, show plain text or an indicator in its place rather than a link that breaks on click. The no-quantity attendee's ticket cell is the reference: a quantity-0-only attendee has no live `/t` page (it 404s), so admin views render a "No quantity" indicator instead of the `/t` link. This holds for permission-gated links too: an action a role can't reach must not be linked for that role. Mind the blind spot — a link to a restricted page still works when the page is viewed (or tested) as a high-privilege user, so the dead link the lower-privilege roles see goes unnoticed. Gate the link on the same permission the target enforces, and when testing visibility, render the page as each role rather than only the most-privileged one.
 - **Operator decides genuine conflicts — a required choice, never a silent default**: When an action hits a conflict the system cannot unambiguously resolve (e.g. an attendee merge where both records booked the same listing, or where each side carries a real payment), do NOT auto-pick a resolution and quietly proceed. Surface the conflict and make the operator choose explicitly via a **required** field — the request fails closed until they decide. Silently moving money, voiding a leg, or keeping one side by default hides a real decision behind a guess; an explicit operator choice keeps the irreversible call — especially anything that touches the money ledger — with the human who can see the context.
+- **Never hold a seat**: A checkout NEVER reserves capacity. A staged booking is written at **quantity 0** (`stageCheckout`), so it claims no seat while the customer pays; the real quantity — and the authoritative capacity claim — lands only at activation, after the payment confirms. First payment to land wins; a second buyer who pays for the genuinely-last seat is refunded by the terminal refund path (rare, and cheaper than the alternative). We do **not** reserve because holding seats invites botting and ghost-checkout lockouts on exactly the scarce listings where it hurts most. The availability preflight (`checkBatchAvailability` in `createStagedCheckout`) refuses a checkout that already can't fit, but it *holds* nothing. This is policy: never add a "reserve this seat for N minutes" mechanism.
 - **Select only needed columns**: Avoid `SELECT *` and broad "load every row" helpers — query the specific columns a caller actually uses. See [Database Queries](#database-queries).
 - **SQL table aliases**: Alias tables with the full singular word using `AS`, not a single letter — write `FROM listings AS listing`, never `FROM listings e` (the `e` is a leftover from when listings were called "events"). When one query references the same table more than once (e.g. correlated subqueries that compare a row against its group), give each occurrence a descriptive word alias — `listing` for the row being checked, `groupListing` for sibling rows in its group.
 - **Annotate return types on exported functions, and keep types easy to compile**: Give every exported/public function an explicit return type instead of leaning on inference. A named annotation is more compact for the checker to record than a re-inferred anonymous type, and it fails loudly at the definition when the body drifts from the contract rather than leaking a surprising shape to callers. This is the [TypeScript performance guidance](https://github.com/microsoft/TypeScript/wiki/Performance) applied to our checker (`deno check` is the same compiler underneath): prefer an `interface`/base type that others extend over a large `type X = A & B & C` intersection or a wide bare union (comparing many members is quadratic), and give a complex conditional type its own name so the compiler caches it instead of re-deriving it at every use. A small two-way `A & B` merge, or a `v.variant`/discriminated union built from the schema-first patterns above, is already the right shape — this is about not hand-rolling sprawling anonymous ones. (The wiki's `tsconfig`/project-reference/tracing advice does not apply: we type-check with `deno check`, not `tsc`.)

TODO.md

@@ -338,26 +338,14 @@ existing ticket?) and likely adding payment-intent uniqueness — out of scope f
 a test-only file split. Starting point: `src/features/api/payment-processing.ts`
 (the `/payment/success` finalize path) and `#shared/db/processed-payments.ts`.
 
-## Payment-processing review follow-ups (from PR #1692)
+## Payment-processing review follow-up (from PR #1692)
 
-Both items describe behaviour that predates the payment-processing split (the
-code was moved verbatim from the old `payment-processing.ts` monolith). They are
+This item describes behaviour that predates the payment-processing split (the
+code was moved verbatim from the old `payment-processing.ts` monolith). It is
 recorded here because the split PR was a pure reorganisation — changing this
-behaviour there would be out of scope — and CodeRabbit flagged them as worth a
-look.
-
-- **Refund after a committed booking** (`src/features/api/payment-processing/index.ts`,
-  the `try { honoured = await createAttendeeForSession(...) } catch` in
-  `processReservedSession`). `createAttendeeForSession` commits the attendee +
-  bookings atomically, then runs `ensureAllBookings` (a post-commit read). If
-  that post-write step *threw*, the `catch` would route to `storeRefundedBooking`
-  — refunding a booking that actually persisted. Today `ensureAllBookings`
-  returns a structured `{ ok: false }` rather than throwing on the capacity path,
-  so the window is theoretical, but it isn't guarded structurally. Fix direction:
-  narrow the `try` to the pre-commit call only, or guarantee the post-commit
-  cleanup path is non-throwing, so a persisted booking can never be refunded.
-  Add a regression test that makes the post-commit step throw and asserts no
-  refund is issued.
+behaviour there would have been out of scope — and CodeRabbit flagged it as
+worth a look.
+
 - **Per-item DB reads not batched** (`src/features/api/payment-processing/items.ts`
   `validateAllItems`, and `package-pricing.ts` `loadPackagePricingByGroup`).
   `validateAllItems` calls `getListingWithCount` once per item in a loop, and
@@ -1006,3 +994,47 @@ Follow-up: give each variable's description its own locale key under a new
 (these are developer/operator reference docs for a technical feature, and the
 i18n-coverage gate doesn't flag them), but worth doing when this settings surface
 is next touched. Keep the copy plain per the Simple-Language rules.
+
+## Deferred from the staged-checkout hardening PR (review of PR #1764)
+
+*Origin: the adversarial review of the staged-checkout branch. These were
+verified as accurate findings but deferred because each trades a small win
+against real churn or a redesign of a money path; the hardening PR stayed
+scoped to the confirmed bugs.*
+
+- **Batch `activateStagedBooking`'s interactive transaction.** It holds the
+  primary write lock across one round-trip per statement (pii update, N booking
+  updates, M usage inserts, legs, finalize, stage mark) plus in-transaction
+  crypto (`batchFinalizeStatement` encrypts inside the lock). The batch-with-
+  guards shape in `create-batch.ts` (`BOOKING_WRITE_GUARD`-style `changes()=0`
+  probes instead of `rowsAffected` checks) would make the whole activation one
+  round trip. Substantive rework of a money path — do it with its own focused
+  mutation run.
+- **Thread already-loaded listings into `stageCheckout`.** `stagedBookings`
+  re-fetches (and re-decrypts) the order's listings that all four
+  `createStagedCheckout` call sites already hold. Threading them through saves
+  one SELECT per paid checkout start but adds a parameter every future checkout
+  entry must supply correctly, and deletes the "listing vanished before
+  staging" freshness throw — weigh both before doing it.
+- **Batch the two-execute spots.** `store-refund.ts`'s staged path
+  (`updateAttendeePII` then `markCheckoutStage`) and `recordOrderActivity`'s
+  email+phone upserts each fire sequential single executes. Both are
+  replay-healed (verified), so this is round-trip thrift, not correctness; it
+  needs the helpers restructured to return statements instead of executing.
+- **One shared "session returned unpaid" resolver.** The cancel path discards
+  on `paymentStatus !== "paid"` while classify discards only on `"failed"` —
+  deliberate today ("unpaid" can still complete on the success path), but the
+  next provider integration should not have to rediscover that asymmetry.
+- **Reuse #1775's `createSystemNoteOnce` for stranded-refund note dedup** once
+  that PR merges — the store-refund replay path can stack an identical system
+  note on a redelivery after a mid-flight crash (rare, replay-healed).
+- **`defineTable`-ize `checkout_stages`** (hand-written row type + mapping in
+  `checkout-stages.ts`) when the table next grows a column.
+- **Rebuild refund placeholders from the signed lines on `stage_mismatch`.**
+  A refused staged order keeps its rows as stored; with the admin guard those
+  rows can no longer be operator-edited while pending, so the realistic
+  mismatch sources are derived-key changes (listing type flips, parent-edge
+  changes) where the rows still name the customer's exact listings and
+  quantities. If a real divergence source ever appears, re-write the
+  placeholder rows from the signed intent before refunding, so the operator
+  record always mirrors what was paid for. (Raised by Codex on PR #1802.)

plan.md

@@ -0,0 +1,107 @@
+# Plan: staged-checkout money model
+
+This branch (`claude/branch-review-comparison-pcwbvd`, PR #1802) now targets
+`main` directly and already contains the whole staged-checkout system: the
+original P1–P9 hardening (see git history / the PR description) **plus** the
+post-review work below. Everything here ships with regression tests that fail
+before the fix (repo rule) and a green `deno task precommit`.
+
+---
+
+## Shipped
+
+- **P1–P9** — the original hardening of the staged flow (terminal paths,
+  pending-stage guards, expiry/retention, polish). Merged into this branch.
+- **Review item 1 — a gone stage is an impossible state, not a recovery.**
+  `findStageProblem` (zero rows) and `getCheckoutStageOrNull` (dangling pending
+  stage) now throw instead of booking fresh around a missed cascade. Dropped
+  `stage_gone` and the `createFresh` recovery (kept `createFresh` for the
+  genuine no-stage path). To earn the throw: admin delete of a mid-payment
+  record is blocked, listing deletion is blocked while it has a pending checkout
+  (`listingDeleteError` on all three delete paths), and the orphan purge
+  cascades `checkout_stages`. Fixed the "or delete the record" copy.
+
+---
+
+## Item 6 — validate before the provider (shipped, slice 1)
+
+`createStagedCheckout` now fully validates the order **before** creating the
+provider session: the real quantities must fit and every listing must be on sale
+(`checkBatchAvailability`), or the customer is told up front
+(`public.checkout_unavailable`) and never sent to pay. It is the single
+chokepoint every new-booking entry point routes through, so no sold-out or
+off-sale order can reach the provider. The dead balance-attendee branch was
+removed (only new bookings run through here). Committed with refuse tests for an
+overbooked and an off-sale listing.
+
+## Item 3 — staged money model: DECISION (keep legs at activation)
+
+The locked design was "owed legs at staging, received legs at activation." On
+deep inspection this conflicted with the ledger's core invariant and bought
+nothing, so **the operator chose to keep all legs at activation** — no
+owed-legs-at-staging, no two-group split. Why:
+
+- **The ledger posts an event group as one immutable set.** `assertEventMatches`
+  rejects a replay that adds or omits a leg, so you cannot post owed legs at
+  staging and grow that group with the payment leg later. Owed-at-staging would
+  have to be **two** event groups (the balance-payment pattern).
+- **Per-row `price_paid` is sale-scoped to the row's single stamped
+  `ledger_event_group`** (`select.ts` `pricePaidFromLedger`), and the
+  "incomplete sale" detection assumes the payment leg shares that group
+  (`listing-overview-stats.ts`). A two-group split needs careful handling to
+  avoid misreading paid bookings.
+- **A pending stage is excluded from sums for free.** Because activation posts
+  every leg and staging posts none, a pending staged booking has **zero** ledger
+  legs — it contributes nothing to any sum, and its quantity-0 rows claim no
+  capacity and show no money. So the totals are identical to the owed-at-staging
+  design, with none of the rework.
+- **Activation is already atomic** (one transaction: set quantities + post all
+  legs + finalize payment + flip stage), so there is no partial-state bug to fix.
+
+Consequences: slices 2–4 (owed-at-staging, exclude-pending-from-sums,
+delete-legs-on-abandon) are **unnecessary** and dropped. A pruned/abandoned
+pending stage just deletes its quantity-0 attendee + rows (no legs to delete).
+
+### We never hold a seat (still document in AGENTS.md)
+
+Quantity-0 staging means a checkout **never reserves a seat**. First payment to
+land wins; a second person paying for the genuinely-last seat is refunded (rare,
+handled by the refund path). We do **not** reserve because holding seats invites
+botting and ghost-checkout lockouts on exactly the scarce listings where it
+hurts most. This has come up repeatedly — it is policy.
+
+### Remaining item-3 Codex findings
+
+- *"paid stage pruned as abandoned"* — the prune only ever deletes stages still
+  `pending`; a paid stage has flipped to `booked`, out of the prune's reach.
+  Verify a direct test covers this.
+- *"deleted-listing refund reuses an empty staged attendee"* (store-refund.ts) —
+  the delete/stage race can strand a paid order on a staged attendee whose rows
+  were cascaded away. Restore the signed ghost rows (or close the race).
+
+---
+
+## Remaining review items (not yet started)
+
+- **#2** stage transitions as compare-and-set (session id + attendee id +
+  `state='pending'` + exactly-one-affected-row).
+- **#5** rewrite `activateStagedBooking`'s input-sized interactive transaction as
+  a guarded batch (create-batch.ts pattern).
+- **#7** make validation side-effect free (return a typed refusal).
+- **#8** admin lifecycle: hide edit/logistics/merge/resend/SMS/email while
+  pending (delete now joins this list — it's blocked server-side but the button
+  is still rendered); "Checkout pending" wording; pending state in tables/CSV;
+  owner-gate the ledger link in refund notes; the no-quantity-edit-strands-cash
+  fix.
+- **#9** provider lifecycle: expire remotely before local discard; SumUp
+  EXPIRED; reconcile existing Stripe endpoints; queryable stage relation.
+- **#10** repo cleanup: don't export raw `DEPENDENT_ROW_TARGETS`; split large
+  files; remove any remaining Stripe alias/test-only exports; keep this file
+  current.
+
+## Verification
+
+- Per slice: targeted `deno task test:files` on the touched suites.
+- Per commit: full `deno task precommit` (typecheck incl. tests, strict lint,
+  full suite, 0% duplication, 100% coverage).
+- Rewrite the PR description in plain language when the money-model change lands.

scripts/mutation/equivalent-mutants.txt

@@ -815,13 +815,12 @@ src/features/admin/attendees-merge.ts:188:40  && → ||   # keptIsPinned = Boole
 src/features/admin/attendees-merge.ts:344:54  target → ""   # parsePiiDecisions fallback string: every consumer of decision.pii (pickPiiField, the kept/address ternary on :186, mergedPiiName on :319, applyPiiDecisions on attendee-merge.ts:501) only tests `=== "source"`, so any non-"source" value (including "" and "target") is treated identically as "keep target"
 src/features/admin/attendees-merge.ts:374:15  skip_source → ""   # toBookingChoice comparison `raw === "skip_source"` → `raw === ""`: parse "skip_source" as the "keep_target" fallback. Unobservable — both fall to applyBookingDecisions' same `else` branch (no DB write, bookingsSkipped++) and discardedSaleAmount returns sourceSaleAmount for both, so the merge outcome is identical for every input
 src/features/admin/attendees-merge.ts:408:10  return undefined → return undefined   # toMoneyChoice's fallback is already `return undefined`; the mutation is a no-op (it produces identical code), so no test can ever observe a difference
-# contact-tokens.ts — `?? → ||` and `return null → return undefined` sites whose
+# contact-tokens.ts — `?? → ||` sites whose
 # operand can never be a falsy-but-non-null value, or whose result is only ever
 # consumed by a nullish check. Each is provably equivalent from the types.
-src/shared/db/contact-tokens.ts:92:30  - → +    # splitTokenBlob's `separatorAt === -1`: every app-written line has a tab at the marker-length position (64), and a malformed separator-less line's first-chars marker can never equal a real BlindIndex (an hmac hex), and its ciphertext never decrypts, so taking the else-branch for a tab-less line (marker = raw.slice(0, -1)) is unobservable through ensureBookingToken/removeBookingToken/getRecentBookingTokens
-src/shared/db/contact-tokens.ts:177:35  ?? → ||    # loadTokenBlob's `row?.attendee_tokens_blob ?? null`: attendee_tokens_blob is string|undefined; the only falsy-non-null is "" and tokenLinesFrom("") === tokenLinesFrom(null) === [], so ?? and || agree
-src/shared/db/contact-tokens.ts:218:22  return null → return undefined    # removeBookingToken's no-match return; removedSource is only ever consumed by `removedSource ?? sync.source`, and null and undefined agree under ??
-src/shared/db/contact-tokens.ts:289:18  ?? → ||    # `removedSource ?? sync.source`: removedSource is BookingSource|null, and both BookingSource values ("admin","public") are non-empty truthy strings, so it is never falsy-but-non-null; ?? and || agree
+src/shared/db/contact-tokens.ts:97:30  - → +    # splitTokenBlob's `separatorAt === -1`: every app-written line has a tab at the marker-length position (64), and a malformed separator-less line's first-chars marker can never equal a real BlindIndex (an hmac hex), and its ciphertext never decrypts, so taking the else-branch for a tab-less line (marker = raw.slice(0, -1)) is unobservable through ensureBookingToken/removeBookingToken/getRecentBookingTokens
+src/shared/db/contact-tokens.ts:174:35  ?? → ||    # loadTokenBlob's `row?.attendee_tokens_blob ?? null`: attendee_tokens_blob is string|undefined; the only falsy-non-null is "" and tokenLinesFrom("") === tokenLinesFrom(null) === [], so ?? and || agree
+src/shared/db/contact-tokens.ts:286:18  ?? → ||    # `removedSource ?? sync.source`: removedSource is BookingSource|null, and both BookingSource values ("admin","public") are non-empty truthy strings, so it is never falsy-but-non-null; ?? and || agree
 
 # attendees/tokens.ts — `?? → ||` sites where the operand is a Map#get() whose
 # value is always a truthy object or array (never a falsy-but-non-null value),