Cloud code review — Salus-Website pull request 18

The web checkout and the reconciler, whole.

The pull request is REVIEW ONLY and is never merged. The fixes below are committed to main instead, exactly as pull request 17's were.

The verdict

The web checkout takes a caller's word for who is buying: two checkout functions read user_id straight out of an unauthenticated body, and the rest of the findings are a lifetime purchase that leaves a Stripe subscription billing, a transient lookup read as an absent account, a dedupe marker written through a swallowed insert, an attribution column renamed out from under a swallowed error, and four smaller faults of scope, bound and duplication.

The findings, money first

Ordered by what can lose money, entitlement or a buyer's identity, not by the order the reviewer wrote them. severity is the reviewer's own word mapped to the board's vocabulary: normal becomes medium, nit becomes low, and the one pre_existing finding is medium with raw_severity keeping the word. Nothing was upgraded or downgraded on the way through.

Where the record lives

2026-09-04-pr18.json beside this file is the machine record, in the shape README.md declares, and it carries the reviewer's untouched output as raw_findings. Each finding's outcome and closed_by say what happened to it.

What was done about them

Fixed at the cause, each with a failing test written first: findings 1, 2, 3, 4, 5, 6 and 7. Deferred with a reason: findings 8 and 9 (see the outcome field in the JSON).

The failing test first. supabase/functions/_shared/review-pr18-wiring_test.ts, run against the unmodified handlers before a line was changed: 0 passed, 16 failed, at least one per finding.

Proof it can fail. supabase/functions/_shared/review-pr18-redproof.sh plants 43 faults, one at a time, and requires the suite to go red for each. 43 planted, 43 killed, 0 survived, against a healthy control that must be green before any mutation counts. Six mutations survived an earlier run and every one was a defect in the PROOF, not in the fix: an anchor that survived a rename, a name that survived inside a guard condition, the same name surviving inside an import line, a short-circuit inserted before a checked call, and a test uuid made only of digits so an upper-case mutation was the same string. All six were repaired before this was called done. The whole Deno suite is 463 green.

What the Council changed. The first run (WARN, 1 PROCEED / 3 WARN) agreed the fixes were right and objected to one decision: a request that NAMED an account with no bearer to prove it was being refused 401, and a ten-minute HTML cache, a stale tab or an expired access token produces exactly that for a real signed-in buyer. Three seats of four said the same thing independently. It is answered in the code: the name is now DISCARDED and the purchase proceeds as a guest, which closes the hole just as completely (nothing is attributed to the named account, so no cancel of theirs can follow) and never refuses a buyer. A mismatch is still refused 403. Three smaller objections were answered in the same pass: the owner-lookup retry is bounded by an attempt ceiling as well as by the clock, because retryCeilingReached(null) is deliberately false for ever; Stripe's own resource_missing code is preferred over its prose; and a standing schema fault is suppressed to one alarm an hour so it cannot bury every other sentinel.

The audit the panel asked for. stripe-webhook calls resolveUserId in exactly three places (lines 972, 1056 and 1134) and its uuidCandidates are only ever session.client_reference_id and sub.metadata.user_id — the two fields these checkout functions write, and both are now written from the verified id alone. No other caller-supplied identifier reaches it; its remaining fallbacks are the existing subscriptions row and the Stripe-verified email.

The second Council round, answered (4 September 2026)

salus-brain/reports/council_web-checkout-review-fixes_2026-09-04-1010.md returned WARN (4 WARN, deepseek silent in both rounds) and its synthesis named two items as blocking before deploy. Both are answered here, neither was handed to anybody.

Blocking item 1: "prove the write sites." The panel could not see, from the diff, where session.client_reference_id and sub.metadata.user_id are actually assigned, and that seam is what the whole identity fix rests on. Measured by reading the tree, not by assertion:

• stripe-checkout/index.ts:352 is the ONLY place buyerId is bound, and it is bound to identity.userId — the verified id or null, never the request body. Line 353 derives hasUser from it.
• The only writes are lines 400 (if (hasUser) params.client_reference_id = buyerId;), 411 and 424 (user_id: hasUser ? buyerId : "guest" in the subscription and payment-intent metadata). There is no fourth write and no other source.
• create-checkout/index.ts:243 binds buyerId the same way; line 246 builds the PayPal custom_id from it and line 265 is the only write of that field.
• client_reference_id is written in exactly ONE place in the entire codebase (stripe-checkout:400), and read in exactly two (stripe-webhook:972 and :1056, the uuidCandidates of resolveUserId). No caller-supplied identifier reaches it.

Blocking item 2: "bound the retry when the counter store is unreadable." This was a real defect and it is fixed at the cause, with the failing test written first. retryCeilingReached answers false with no usable clock ON PURPOSE, and rateLimitHit answers null when its own store is unreadable — so when a capture carried no usable create_time AND the counter was down, both halves of the bound were false for ever and lifetime-order would have answered 503 to the buyer's page on every attempt until a human noticed. A counter outage reopened the exact unbounded loop the attempt ceiling had just closed.

• New reader: _shared/owner-lookup.ts exports retryClockUsable(), the same parse retryCeilingReached performs, said once so the two cannot drift.
• New branch: lifetime-order/index.ts — if (attempts === null && !retryClockUsable(capturedAt)) raises one money alarm naming the order and answers needs_manual, so the payment is reconciled by hand rather than retried for ever. It is never dropped into guest resolution.
• Tests first, red before green: four cases in _shared/owner-lookup_test.ts (red: "does not provide an export named 'retryClockUsable'") and one branch-anchored assertion in review-pr18-wiring_test.ts (red: wiring 3 FAILED). Anchored on the whole branch, never on the name, because the name also appears in the import line.
• Suite: 467 green, 0 failed. Red-proof: 44 planted, 44 killed, 0 survived, the new mutation being "let a counter outage reopen the unbounded 503 loop".

Deployment, and the first run on the live path. The fixed functions were NOT deployed when this round began: the deployed stripe-checkout was version 26 of 2026-09-04T00:47:08Z and did not carry decideCheckoutIdentity at all. Deployed through the project's own route (supabase functions deploy <fn> --project-ref egywowuyixfqytaucihf, verify_jwt taken from supabase/config.toml and no flag typed by hand — all three pinned false) and the DEPLOYED BYTES were then read back independently:

First-run proof (live path, watched): at 2026-09-04T07:35:40Z a POST to the live stripe-checkout naming user_id 00000000-0000-4000-8000-000000000001 with no bearer token and a valid plan returned HTTP 200 with a Stripe session, not 401. That request reaches decideCheckoutIdentity (plan validation is line 311, the identity decision line 341), so the deployed code took the downgrade-to-guest branch on a live request: the exact behaviour the first Council round demanded, proved on the live function rather than in a test. No money moved; an unused Checkout Session expires on its own. An earlier probe at 07:33:58Z with an invalid plan returned 400 from both stripe-checkout and lifetime-order and proves only that the deployed bytes execute — it stops at line 311 and is recorded as such.

Named debts, declared rather than passed off.

1. The lifetime-order bound is proved by unit test, mutation kill and deployed-byte read-back. It is UNPROVEN on a live capture: exercising it needs a real PayPal capture whose create_time is unusable while the rate-limit store is down, and no route creates that without a real payment.
2. The Stripe session created by the live drill was not read back through the Stripe API, so "the downgraded session carries no client_reference_id" is inferred from the deployed source (line 400 is guarded by hasUser), not measured at Stripe.
3. The Council's non-blocking recommendations remain open and are recorded here rather than silently dropped: a counter/metric for downgraded checkouts, a once-per-id/day diagnostic on Stripe resource_missing, a test for a downgraded signed-in buyer with an email mismatch, and a test that ad_attribution reattaches post-webhook in the downgrade case.