Make the payment aggregate base build, pass its tests, and be safe to split up - #1962
Make the payment aggregate base build, pass its tests, and be safe to split up#1962stefan-burke wants to merge 196 commits into
Conversation
Each provider now resolves webhook sessions authoritatively rather than trusting stale cache state. A provider-neutral 'retry' signal lets a webhook tell the HTTP boundary to return 503 so the provider redelivers when payment data is temporarily incomplete. Stripe: retry when a paid webhook lacks payment_intent SumUp: retry when a paid checkout lacks transaction_id Square: use the exact webhook payment id, validate against order/amount/ currency/status/refund state; retry on temporary provider data gaps instead of falling back to stale tender data Square local terminal replay is checked via processed_payments and the transfers ledger (hasTerminalPaymentOutcome). Pure Square payment validation lives in square-payments.ts; payment-outcome.ts checks terminal outcome state. WebhookSessionResolution type added to PaymentProvider interface. handlePaymentWebhook exported for direct testing. WIP: webhook handler mutation tests not yet green — log spy assertions need fixing before precommit.
Expand test/features/api/webhooks.test.ts from 1 to 29 tests covering all webhook resolution paths (retry/skip/unrecognized/unverifiable/unpaid), signature failure, missing provider, already-processed redirect with thank-you URL preservation, token-clearing redirect, explicit thank-you override, hidden package suppression, deleted listing, multi-listing suppression, and param fallback logging. Extract test helpers (stripeWebhookResponse, stubRetrieveSession, createSoldOutListing, expectAcknowledgedWithDebug, expectRejected400, expect400With, expectInvalidCallback) to eliminate all jscpd duplication. Add equivalent-mutant entry for webhooks.ts:169 (= → +=) — the if guard guarantees thankYouUrl is empty when the line runs. Add refunds.test.ts covering the empty-paymentReference early return (good-citizen coverage fix). Add square-payments test for tenders without a paymentId.
- square-provider: only skip fully refunded payments (refundedAmount >= amountTotal); partial refunds now process as paid so the booking is honoured - stripe-provider: fetch the session via retrieveSession before returning 'retry' when the webhook snapshot lacks payment_intent, so a session that has since been populated can be processed - sumup-provider: return 'retry' instead of null when retrieveCheckoutById returns null for a checkout we confirmed is ours (transient API failure) - square-payments: reject payments whose amount differs from the order total; add explicit return type to findCompletedSquarePayment - payment-helpers: extract hasPaymentReference shared helper, replacing hasExpectedPaymentReference (stripe) and the inline check (sumup) - square-provider: move stale 'skip silently' comment to above completeSquareOrder where the skip actually occurs - Update all affected tests; add regression tests and direct isPaymentRefunded tests for stripe-provider; achieve 100% mutation kill on all modified sources - Clean up stale equivalent-mutant entries (pre-existing)
CodeRabbit suggested alerting on repeated retry failures (PR #1905). Out of scope for the PR — recorded with context for future pickup.
- Thread 1: Split webhooks.test.ts into resolution + callback-params files - Thread 2: Drop test-only export of handlePaymentWebhook; tests use routeRequest - Thread 3: Square partial refund returns new partial_refund state, skip with log - Thread 4: Stripe resolveWebhookSession distinguishes transient (retry) from permanent (acknowledge) null sessions by checking fetched session metadata - Thread 5: Square findCompletedSquarePayment returns FindCompletedPaymentResult union with no_completed_payment (transient) vs invalid_payment (permanent) - Fix assign-built-site test env leak from shared isolate group - Fix coverage gaps: Stripe null-fetch retry branch, helpers null route branch - Add 2 equivalent mutants for square-payments.ts - Record 5 pre-existing webhookResultResponse survivors in TODO.md
The aggregate rewrite renamed and moved production code without updating every test that used it, so `deno check src test cli scripts` failed with 67 errors while `deno check src` alone passed. Renames applied: stripeApi.refundPayment is requestRefund, the provider's refundPayment is refundCharge, Square's retrieveOrder/retrievePayment are readOrder/readPayment, and BULK_REFUND_LIMIT is the derived BULK_REFUND_FOREGROUND_REFERENCE_LIMIT (5 became 3). Restored the deleted payment-processing/index/helpers.ts with the two helpers its three importers still need, pointed refund-header-probe at the stripe fixtures that actually exist, and added the assignment_effect field the built-site fixtures were missing. Removed tests whose subject no longer exists, only where the behaviour is covered elsewhere: tryRefund (covered by payment-runtime/refund.ts tests), parseWebhookPayload and validatedPaymentSession, the resolveWebhookSession and retrieveSession fallbacks (covered by resolve.ts and the webhook resolution tests), and an already-refunded route case that now resolves as an ordinary completed refund. Moved the zero-value promo check to the webhook promo-code tests, where it runs against the real completion effect that formats the message, so the "+£0" versus "£0 off" boundary stays covered. Deleted the duplicate withRefundMock in the specs support folder in favour of the migrated one.
…in tests The rewrite dropped the registration_closed refund reason: every booking that failed validation for a reason other than a missing listing was filed as price_changed. A listing that closed, or stopped taking bookings, told the operator "the listing price changed while they were paying", which is not what happened, and no code emitted registration_closed at all. Restored the reason and picked it from the validation outcome, so a closed or no-longer-accepting listing (410) is told apart from a real price change. The webhook tests for a closed listing are the regression: they recorded the wrong reason before this change and the right one after. Refund test doubles now answer with a refund that genuinely belongs to the charge. The provider checks that a refund matches its charge on payment reference, amount and currency, and the doubles still returned only an id and a status, so every refund path threw. The reference now comes back from the request itself, and the amount comes from the same fixture that sets what the buyer paid, so the two cannot drift. Added a helper to finish the work a paid callback defers. Only the provider refund happens while the buyer waits; the note, ledger entries and activity are written by scheduled maintenance, so a test that checks any of them now settles that work first, as the real site does moments later. Moved the refund reason assertions onto the note that carries them, since the webhook reply now reports only that the payment was refunded, and corrected a multi-listing test that expected the open listing's booking to be dropped when the design keeps it at quantity 0.
The redirect path resolves a payment through lookupCheckoutSession and then lookupPaymentIntent, but the test doubles still answered the retired retrieveCheckoutSession. Nothing stubbed the calls the code makes, so the real ones ran, the session could not be proven ours, and every redirect ended on "We could not find this payment" without refunding a penny. The shared double now answers both reads, with the intent and its charge mirroring the session's reference and amount, so the money the provider reports matches what was paid. The webhooks helper delegates to it instead of keeping a second copy that had drifted the same way. Test expectations updated where the behaviour deliberately changed, each checked against the code rather than assumed: - A refunded checkout answers the buyer with 200 and one plain message saying the money went back and the organiser can help. The reason it happened is kept on the booking's note for the organiser. - A listing that fills up while someone is paying is an ordinary race, not a fault, so nothing is written to the error log. The refunded booking on that listing is what makes it traceable, so the test checks that instead.
…m the catalog A paid callback finishes the provider refund and its note in scheduled maintenance, so the shared "refunded with a note" check now settles that work first. Without it the note was missing and the refund looked as though it had never been requested. Payment error assertions now come from the message catalog instead of repeating the words. The rewrite reworded these into plainer English, and a test that hard-codes the old sentence breaks on wording alone, which is exactly what the catalog exists to prevent. A booking refused because its listing stopped taking registrations now answers the buyer the same way every returned payment does: 200, with the plain message that the money went back. The reason stays on the booking's note for the organiser.
The already-processed test seeded the old processed_payments table through a helper that no longer exists, so it could not even load. It now records the same thing as a completed payment record, which is what a replay reads today. Three webhook cases expected the request to blow up when a provider's answer broke its own schema. That is no longer what happens, and the new behaviour is deliberate: the answer is rejected before the signed proof inside it can be read, so the payment cannot be shown to be ours and is left alone rather than booked or refunded. The tests now assert that, and say so in their names. The gap this leaves — real money can be dropped silently when a checkout has no local payment record behind it — is written up in TODO.md rather than quietly accepted, since closing it needs a decision about how to record a dropped callback without letting strangers create noise. Also switched the last two inline doubles onto the shared one, so every test answers the reads the provider actually makes, and corrected a multi-listing rollback expectation the same way as the other one: the open listing keeps its booking at quantity 0 instead of losing it.
A paid callback books the ticket and leaves the rest — saved answers, activity entries, outgoing messages — to scheduled maintenance. Tests read those the moment the callback returned, so answers came back empty and activity lines were missing. The shared processed-callback check now finishes that work, the way the site does a moment later. The session double also answers a free checkout properly: nothing was paid, so there is no payment behind it and the status says none was required. Stubbing a zero-value payment made the provider read reject its own answer. Swapped the last inline session double in the price tests for the shared one, and removed the loud-failure helper now that no test asks for a callback to blow up.
The refund activity entry names the reason code and says the booking was kept, rather than repeating the message shown to the buyer, so the logging tests read that. The console line they also watched for is gone with the helper that wrote it; what those tests actually prove — the refund was asked for once, against the right payment, and recorded against the listing — is still checked. Remaining payment error assertions now read the message catalog, one stale "Processed payment" wording follows the record's new name, and a refund double returns the amount its charge actually carries.
The rewrite added a strict shape for the free-text answers carried through a checkout, requiring the stored-string id on every one. A single answer whose id went missing therefore failed the whole booking's shape, so the payment could not be shown to be ours and the callback quietly did nothing: money taken, no booking, no refund, nothing written down. The code that handles this properly was still there and still commented "the payment is already captured, so the booking must still finalize" — it just could not be reached. The shape now allows an answer with no id, and that existing step drops the one answer and says so, which is what the branch this came from did. The test that proves it reads the error log rather than the activity log, because the activity write is best-effort and is swallowed when it cannot run. Also settled deferred answers in the two question suites that post callbacks directly, matched a webhook reply assertion to the reply's new shape, signed the unpaid-checkout fixture so it exercises the unpaid path rather than an unprovable one, and updated two signature-check messages to what the route now returns.
A paid checkout the provider has not attached a payment to is now a case for the owner rather than a retry, so the test says that and the notice fixture can describe a payment with no charge behind it. A refund the provider refuses still asks to be sent again, which is the point of the old no-provider test: acknowledging would leave the buyer charged with nobody looking. It now makes the refund fail directly instead of counting calls to a lookup the refund no longer makes. Renewal pushes are sent by maintenance after the redirect, so the tests that check one settle that work first.
…utcomes A SumUp checkout that cannot be read at all now asks for redelivery instead of being waved through, which is safer than dropping a real payment while the provider is down; a checkout it reports as missing is still acknowledged. The concurrency test waits for the winner's booking to be finished off before reading the payment's state, since that last step is left to maintenance.
…ger makes Validating a paid order's items now only reports what it found; returning the money is the caller's job. The unit tests around it asserted a refund had gone out and carried the old result shape, so they checked a step that had moved. They now assert the decision itself. That a closed or withdrawn listing really does get refunded is covered by the callback tests.
… the catalog The double for a cancelled checkout left its details unsigned, so the page could not tell the checkout was ours and answered that it could not find the payment instead of offering the way back to the listing. It is signed now, as a real checkout is. A cancelled checkout whose items cannot be read is reported as unrecognised rather than as a missing listing, because without readable items there is nothing tying it to one. A cancel that names a listing which no longer exists still says so.
A hidden value carries a starting block and then the hidden text, and the merged schema checks for both. The fixtures across the payment and migration tests wrote only the text, so every copied row was refused and the quiet insert dropped it without a word — which is what the whole migration-restore chain was failing on. The values that are meant to be malformed, because their test is that a bad one is refused, are left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a5815ee1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const webhookProviderType = (request: Request): PaymentProviderType => | ||
| PAYMENT_PROVIDER_IDS.find((provider) => { | ||
| const header = PAYMENT_PROVIDERS[provider].webhookSignatureHeader; | ||
| return header !== null && request.headers.has(header); | ||
| }) ?? "sumup"; |
There was a problem hiding this comment.
Pre-filter unsigned SumUp checkout IDs locally
Any unsigned request without a Stripe or Square signature header is classified as SumUp here. SumUp's verifier only parses the supplied JSON, so an attacker can submit arbitrary checkout IDs and make the later reconciliation call retrieveCheckoutById for each one. This removes the previous local hasSumupCheckoutId gate and lets unauthenticated traffic consume SumUp's API quota, potentially blocking real payment callbacks; reject IDs that are not attached to a locally stored payment before calling SumUp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3659845831, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| if (site.readOnlyFrom !== delivery.readOnlyFrom) { | ||
| if (site.readOnlyFrom !== delivery.previousReadOnlyFrom) { | ||
| throw new Error("Renewal deadline changed after payment"); |
There was a problem hiding this comment.
Serialize concurrent paid renewals for the same site
If two buyers pay to renew the same site before either delivery runs, both deliveries store the same previousReadOnlyFrom and target date. After the first applies, the second sees site.readOnlyFrom === delivery.readOnlyFrom and treats its own renewal as already complete, so one paid term is silently lost; with different term lengths it throws instead. Claim the site and extend from its current deadline so every distinct paid delivery contributes its months exactly once.
Useful? React with 👍 / 👎.
| const completed = refunded.filter((candidate) => { | ||
| const ok = posted.get(candidate.attendee.id) === true; | ||
| if (ok) counts.refundedCount++; | ||
| else counts.errorCount++; | ||
| return ok; |
There was a problem hiding this comment.
Queue ledger repair after a provider refund succeeds
When the provider refunds a candidate successfully but recordAttendeeRefundsBatch returns false, this only increments the error count. The payment aggregate is already fully refunded, the candidate is excluded from later refund-all runs, and only successfully posted candidates are passed to resolveQueuedBulkRefundPayments, so the returned money can remain absent from Money permanently with no repair case. Persist retryable ledger-completion work or open an operator case for these candidates instead of merely tallying the failure.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3659845833, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| const created = await client.checkouts.create({ | ||
| amount: Number(toMajorUnits(expected.amount)), | ||
| checkout_reference: localPaymentId, | ||
| currency: v.parse(SumupCurrencySchema, expected.currency), |
There was a problem hiding this comment.
Convert SumUp amounts using the stored currency
When a staged SumUp checkout is retried after the site's currency changes, expected.amount is still in the stored currency's minor units but toMajorUnits uses the current site currency. For example, a stored GBP amount of 1000 retried after switching to a zero-decimal currency is sent as 1000 GBP instead of 10 GBP; the response parser also uses the live divisor, so it can accept the inflated checkout as matching. Convert both request and response amounts using expected.currency rather than mutable settings.
Useful? React with 👍 / 👎.
| const next = pending[0]; | ||
| if (next === undefined) return true; | ||
| await deliverPaymentCompletionData(current, next.id, next.data); | ||
| return pending.length === 1; |
There was a problem hiding this comment.
Escalate permanent completion-delivery failures
If the oldest prepared delivery fails permanently, such as a webhook returning a stable 4xx or stored email settings no longer matching, this await throws before the row is marked complete or actionable. Every retry selects the same first incomplete row, and the exception also aborts the maintenance loop, so later deliveries and later due payments can remain blocked indefinitely. Record a terminal failure or owner case after a permanent refusal so the queue can continue.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3661109294, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| return readProviderOrInvalid(context.payment, context.requested, async () => { | ||
| const account = await resolvePaymentAccount(context.providerType); | ||
| return account.accountId === context.payment.accountId | ||
| ? await context.provider.readPayment(context.payment, context.requested) |
There was a problem hiding this comment.
Persist account-resolution failures before retrying
When credentials are removed or Stripe's account lookup is unavailable after a checkout has been stored, resolvePaymentAccount throws here after reconcilePayment has already claimed the session. The payment keeps its five-minute lease without a new retry result or owner case, and maintenance aborts before later due payments; once the lease expires, the same row can repeat this indefinitely. Store the transient failure as retrying or the unusable account as needing action, then release the claim.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| args: [MIGRATION_PAGE_SIZE], | ||
| sql: `SELECT paymentSessionId FROM (${sources.join(" UNION ")}) | ||
| ORDER BY paymentSessionId LIMIT ?`, |
There was a problem hiding this comment.
Keep both SumUp aliases in the same migration page
Paging the union of raw session IDs can split one SumUp payment's local checkout reference and provider checkout ID across the 25-ID boundary. The first page joins and drains the sumup_checkouts row using whichever alias it saw, but the second page then has no row left to prove that its remaining processed_payments alias belongs to the same payment, so it creates a second aggregate and separates the booking, charge, ticket, or refund evidence. Page by complete merged payment groups rather than cutting the raw alias list at an arbitrary limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3659845838, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| // Move source-owned aggregate payments before deleting the source attendee, | ||
| // so refunds follow the merged person. | ||
| paymentSessionAttendeeChangeStatement( | ||
| { args: [sourceId], sql: "?" }, | ||
| targetId, | ||
| ), |
There was a problem hiding this comment.
Fence attendee merges while paid delivery is pending
When the source attendee has a payment whose booking exists but external completion is still pending, this repoints only payment_sessions.attendee_id and then deletes the source. Existing payment_completion_deliveries still contain the source attendee ID, and an already reserved built_sites row does too, so maintenance can assign or retain the buyer's paid site against an attendee that no longer exists rather than the merge target. Block the merge until completion finishes or repoint the delivery and reserved-site facts atomically.
Useful? React with 👍 / 👎.
The story sold three places and named a payment on each, but who can be refunded is read from the payment record and the money on it, so the page offered nothing to refund. It now makes each place the same way the refund tests do, which also ties the sale in the books to the payment so the refund can reverse it. It no longer sets Stripe up first, on purpose: the refund driver then stands in the same account these payments were made on, and a refund only ever goes out on the account that took the money. This was the last failing story on the branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
Two stories opened the same way — put a thing on sale at a price, then book someone onto it — differing only in who they book. They now share one helper that sells the thing and runs the booking loop, and each says only how its own people are made. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
The fixture could make a payment whose booking was still being finished, but nothing has asked it for one since the payment rewrite, so the lines were never run. Git history has it if it is ever wanted again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
The merge is finished and both suites pass, but the coverage gate does not: 26 payment files the rewrite added are short of the 100% the repository asks for. None was touched by the merge repair, so the list is written down with its starting point rather than left for the next person to rediscover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
|
Why The merge with main is finished. These all pass, each checked by exit code rather than by reading the output:
The one remaining failure is the coverage gate. It is not merge damage. The only Rather than paper over it, the whole list is written down in What the merge repair itself fixed, for the record: the duplicated payment table group in the schema; a stale pre-merge copy of the payment schema test; two migration tests still describing tables that main's Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81982cbc68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw new Error(`Payment ${work.payment.id} has no completion ticket`); | ||
| } | ||
| const ids = unique(plan.input.items.map((item) => item.e)); | ||
| const listings = await getStoredListingsWithCountsByIds(ids); |
There was a problem hiding this comment.
Build delayed fulfilment from the paid snapshot
When the initial callback runs completion in critical mode, every booking effect is deferred because the critical-effects list is empty. If an owner edits the committed booking quantity or changes a listing's site-assignment settings before maintenance resumes, this live reload makes emails, webhooks, and site deliveries use the edited values rather than what the buyer paid for; for example, a quantity-one purchase can allocate two sites. Persist and use the fulfilment facts captured when the booking commits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3660383346, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| await runPaymentDecisionMaintenance(context); | ||
| await runPaymentReconciliationMaintenance(context); |
There was a problem hiding this comment.
Isolate retrying decisions from payment reconciliation
When a due owner decision hits a recoverable provider error, executeAcceptedDecision first persists the decision as retrying and then throws; this first await propagates that error and skips runPaymentReconciliationMaintenance. The same decision becomes due again a minute later, so a provider outage can repeatedly prevent every later checkout, booking completion, and refund from progressing. Report the decision failure without aborting ordinary reconciliation.
AGENTS.md reference: AGENTS.md:L85-L87
Useful? React with 👍 / 👎.
| refundError(attendeeId, NO_PROVIDER_ERROR, returnUrl), | ||
| ); | ||
| if (provider instanceof Response) return provider; | ||
| if (!(await paymentProviderIsConfigured())) { |
There was a problem hiding this comment.
Refund through the payment's stored provider
When the owner disables new checkouts by selecting no active provider, this guard rejects every single-attendee refund even if the credentials for the provider that took the payment are still present. The loaded target already carries its provider and account, and refundPaymentTargets resolves that exact provider, so the equivalent bulk-refund path can still work while this route falsely reports that payments are not configured. Remove the active-checkout guard and let stored-account resolution determine whether this payment can be refunded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and recorded. Setting the active provider to none stops new checkouts, but it should not stop refunding money already taken — the loaded target carries its own provider and account, and refundPaymentTargets resolves that exact one, which is why the bulk path still works while this route says payments are not configured. The two paths disagreeing about the same payment is the part that makes it a bug rather than a policy.
Not fixed here; recorded in TODO.md. Raised four times (#discussion_r3661665861, #discussion_r3677177526, #discussion_r3689865047 for the related pending-refund case, and here).
Generated by Claude Code
| criticalEffects: ["provider_refund"], | ||
| effects: PlaceholderRefundEffectSchema.options, |
There was a problem hiding this comment.
Record captured money before waiting for its refund
When a captured payment cannot be booked and its provider refund remains pending or moves to owner review, making only provider_refund critical pauses the plan before payment_ledger and pending_note. The resulting quantity-zero attendee can therefore remain indefinitely with no captured payment in Money and no note explaining why, even though those writes do not depend on the refund finishing. Run the payment-ledger and pending-note effects before the blocking provider refund.
Useful? React with 👍 / 👎.
| (row) => (row.attendeeId === null) === (row.listingId === null), | ||
| "A completed legacy payment requires one live booking", |
There was a problem hiding this comment.
Migrate paid records whose booking row was deleted
On an existing site where a listing was deleted after payment, processed_payments.attendee_id can still name the retained attendee while the listing subquery returns NULL. This schema check rejects that ordinary historical shape, so parseRows aborts the payment migration and every retry encounters the same row, preventing the site from completing its upgrade. Preserve the payment evidence and create an owner-review case rather than requiring a live booking row.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and recorded — this one can stop an upgrade dead, which makes it the worst of the set. A listing deleted after payment leaves processed_payments.attendee_id naming a live attendee while the listing lookup gives nothing, and the check refuses that shape, so parseRows throws on the same row every retry and the site never finishes upgrading.
Not fixed here. This branch's job is repairing its merge with main so the suite passes, and the fix is a decision about what a payment without a booking should become — evidence kept plus an owner case, as you say — which needs its own regression test rather than being folded into a merge repair.
Recorded in TODO.md. You raised this three times (#discussion_r3661665840, #discussion_r3677177545, and here); the disposition is the same for all three.
Generated by Claude Code
| email: intent.email, | ||
| name: intent.name, | ||
| paymentId: session.paymentReference, | ||
| paymentId: "", |
There was a problem hiding this comment.
Read new payment references from the aggregate
Every newly paid attendee is now stored with an empty payment_id, but the attendee payment panel and CSV transaction-ID column still read that legacy field. Consequently the panel is omitted and every new paid booking exports a blank transaction ID even though the payment aggregate retains the provider reference. Migrate those consumers to the aggregate in this change instead of clearing the old source while internal callers still depend on it.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3658776178, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| >, | ||
| ): Promise<"completed"> => { | ||
| const payment = requireCurrentPayment(context); | ||
| await confirmChargesFullyRefunded(payment.id, decision.charges); |
There was a problem hiding this comment.
Finish the payment after confirming its refund
When the owner chooses the authoritative “already fully refunded” decision, this call updates only the charge rows; the decision then resolves the case without moving the payment session to fully_refunded or recording the attendee's refund in the ledger. The owner sees a completed decision while Money still shows the funds as held, and later callbacks continue to treat the stale payment state as a conflict. Complete the aggregate and ledger repair before closing the case.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and recorded. Confirming the full refund updates the charge rows and then resolves the case, leaving the session short of fully_refunded with no attendee refund in the ledger — so the owner is shown a finished decision while Money still holds the funds, and later callbacks keep reading the stale state as a conflict.
Not fixed here: this branch is being repaired for its merge with main, and completing the aggregate and the ledger inside that decision needs its own regression test. Recorded in TODO.md.
Raised four times (#discussion_r3661665844, #discussion_r3664156011, #discussion_r3677177492, and here) — same disposition for all.
Generated by Claude Code
A till or dashboard payment sends an event with no order on it, which the check refuses, so the site answers 400 and Square keeps resending it. It was the one finding from the reviews with nowhere written down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
The page that shows a case looked its reason up in a table keyed by any text at all, so a reason added to the payment vocabulary compiled perfectly well and then read as "other" on screen — the words and the vocabulary could drift apart without anything saying so. The kinds of thing a case can be about had it worse: an unlisted one produced no words at all. Both tables are now keyed by the words themselves, so leaving one without words does not build. The reason list is built from the schemas that already name conflicts and unreachable providers, plus the four an upgraded payment can need, so it cannot drift from them either. Text read back from an older version is still shown as "other" rather than refused, which is what that fallback is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
…n29f # Conflicts: # test/test-utils/order-journey.ts
Main's new discount-code stories stood in for a provider method this branch renamed, so they now go through this branch's own way of standing in for a checkout. One of the fifteen still fails: the payment goes through but the activity log does not say the code took anything off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
The reason a payment case carries is now checked against the list that names them, so the words shown come from a reason this version really writes rather than an unchecked cast. The list itself is no longer exported on its own, since nothing outside the schema needed it.
Every review thread on the pull request now has a matching entry, so a finding answered on the thread can be picked up later from one place.
The log entry naming the code is written once the buyer has been sent on their way, not while they wait, so the story now settles that work before reading the log — the same wait the other paid stories make.
|
CI status on The Two failures were fixed to get here. The dead-export check was catching a payment-reason list that only a test used, so the reason a payment case carries is now read through its schema instead of being cast, which gives that schema a real caller. And the discount-code story was reading the activity log before the work that writes it had run — the log entry naming the code is written after the buyer is sent on their way, so the story now waits for it the way the other paid stories do. What is left is the payment rewrite's own test coverage — 26 files, listed in Those are the Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99d08010bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (context.payment.completionState === "pending") { | ||
| return fulfilStoredCompletion(context.payment, claim, fulfil); | ||
| } | ||
| if (context.payment.state === "refunding") { | ||
| return retryRefund(context.payment, claim); |
There was a problem hiding this comment.
Retry queued refunds before resuming completion
When refund-all selects a booking whose external completion is still pending, queueing changes the session to refunding, but this ordering resumes the completion first. If the provider refund is pending, maintenance repeatedly tries to finish a refunding payment as completed, the state transition is rejected, and the stored refund is never polled again. Handle the refunding state before pending completion.
Useful? React with 👍 / 👎.
| name: "", | ||
| })), | ||
| }, | ||
| input: redactBookingIntent(completion.input), |
There was a problem hiding this comment.
Preserve balance identity while redacting completion
For a completed balance payment, facts.flow remains balance while redactBookingIntent removes balanceAttendeeId. PaymentCompletionSchema requires those values to agree, so sealing the redacted completion throws. Because the redaction page prepares all statements with Promise.all and pruning awaits that page first, one eligible balance payment blocks all payment-history redaction and the rest of database pruning on every run. Preserve the non-PII balance marker needed by the stored schema.
AGENTS.md reference: AGENTS.md:L796-L796
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and recorded — and your version is worse than how I had it written down, so I have corrected the note. For a completed balance payment facts.flow stays balance while redactBookingIntent removes balanceAttendeeId, the stored shape insists the two agree, so sealing the redacted payment throws. Because the redaction page prepares all its statements together, one such payment stops all payment-history redaction and the rest of the tidying behind it, on every run. That is a standing failure, not a lost marker.
Not fixed on this branch — its job here was repairing its merge with main, and this needs its own regression test. Recorded in TODO.md (sharpened in 3b255f29); you also raised it as #discussion_r3676744923.
Generated by Claude Code
| if ( | ||
| result.ticketTokens.length > 0 && | ||
| currentPayment !== null && | ||
| outcome.status === "completed" | ||
| ) { | ||
| await consumePaymentTicketTokens(currentPayment.id); |
There was a problem hiding this comment.
Consume tokens after the first successful redirect
When the browser redirect itself finishes a payment, runFulfilment returns status fulfilled, not completed, so this condition leaves the aggregate's callback copy of the ticket tokens ready. For the normal token-redirect path, revisiting or sharing the provider return URL can therefore reveal the bearer ticket URL again until a second redirect happens; consume the tokens after the first successful fulfilled redirect as well.
Useful? React with 👍 / 👎.
|
|
||
| /** Resolve every configured choice to the exact non-secret account shown. */ | ||
| export const configuredPaymentAccounts = (): Promise<PaymentAccount[]> => | ||
| Promise.all(configuredPaymentProviders().map(resolvePaymentAccount)); |
There was a problem hiding this comment.
Keep working payment accounts available during outages
On a site with multiple configured providers, a temporary Stripe account lookup failure rejects this entire Promise.all, so loadPaymentCasePage cannot render a legacy repair case even when Square or SumUp is working. The owner consequently cannot assign the usable provider needed to repair the payment until the unrelated provider recovers. Resolve choices independently and omit or mark only the unavailable account.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| if (remaining.size === 0) return { payments: found }; | ||
| if (page === SQUARE_PAYMENT_PAGE_LIMIT) return { issue: "unavailable" }; | ||
| const result = await load({ |
There was a problem hiding this comment.
Fetch Square tender payments by their IDs
When reconciling a Square order from a redirect or maintenance run, the order already provides its tender payment IDs, but this helper scans only eight 100-row location pages. The client requests those pages in ascending order, so once a location has more than 800 older payments, every newer order's tender remains outside the search window and the paid checkout is reported unavailable indefinitely unless an exact-payment webhook arrives. Retrieve each documented tender ID directly instead of paging the location history.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3660115964, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| result === null | ||
| ? creationProgress("created", null, Date.now() + 60_000) |
There was a problem hiding this comment.
Stop retrying permanent checkout creation failures
A null provider result is not necessarily transient: makeProviderCheckout also returns it for a stable malformed response, and withCheckoutError converts every unexpected exception to null. Recording every such result as created schedules the identical create once per minute forever, retaining the encrypted buyer and checkout input and repeatedly calling the provider. Keep transient unavailability distinct from malformed or permanent failures and make the latter terminal or actionable.
AGENTS.md reference: AGENTS.md:L138-L146
Useful? React with 👍 / 👎.
| await deleteListingAttachmentFile(listing, "listing deletion"); | ||
| await deleteListing(listing.id); |
There was a problem hiding this comment.
Claim the listing before deleting its attachment
If a payment completion becomes pending after this pre-check but before deleteListing starts its transaction, the attachment is deleted first and the transactional completion fence then rejects the listing deletion. The listing remains visible with an attachment URL whose file is gone. Take the database deletion claim before removing external storage, or restore/retain the file when the fenced delete fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as #discussion_r3660383372, answered on that thread: it is real, it is recorded in TODO.md, and it is not fixed on this branch — the branch's job here was repairing its merge with main, and this needs its own regression test.
Generated by Claude Code
| AND NOT EXISTS ( | ||
| SELECT 1 FROM payment_sessions AS paymentSession | ||
| WHERE paymentSession.attendee_id = attendee.id | ||
| ) |
There was a problem hiding this comment.
Preserve attendee-only legacy payment references
For an old completed payment whose processed_payments.payment_reference is empty because the provider reference still lives in the attendee record, the first migration pass creates a session attached to that attendee but no provider action. This NOT EXISTS then excludes the attendee from the only pass that reads that stored reference, leaving the copied payment with no charge evidence or owner case and making the historical payment impossible to refund or reconcile. Merge the attendee evidence into an existing copied session instead of skipping it.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| const outcome = await refundCharges( | ||
| payment, | ||
| currentPaymentCharges(payment, context.charges), | ||
| ); | ||
| return outcome.status === "completed" ? "completed" : "retrying"; |
There was a problem hiding this comment.
Record operator refunds in the attendee ledger
When the owner chooses refund_remaining for a payment already attached to a booking, this call can successfully move every remaining amount at the provider and mark the aggregate fully refunded, after which the decision and case are closed. Nothing in this arm calls recordAttendeeRefund, so Money continues to show the returned funds as held and no later worker is scheduled to repair the ledger. Complete the attendee ledger write before reporting the decision as completed.
Useful? React with 👍 / 👎.
|
|
||
| const SquareNoticePaymentSchema = v.object({ | ||
| id: ResourceIdSchema, | ||
| order_id: ResourceIdSchema, |
There was a problem hiding this comment.
Ignore Square payments that do not belong to an order
If the configured Square location also receives a payment.updated event for a dashboard, terminal, or other payment that has no order_id, this required field makes an otherwise signed event fail parsing and the webhook route returns 400. Square then retries an event this application cannot associate with any checkout. Treat the order id as optional at the notice boundary and acknowledge payments that cannot name one of this application's orders.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and recorded. A Square payment.updated event for a dashboard or till payment carries no order id, the check demands one, so an otherwise properly signed event fails parsing, the route answers 400, and Square retries that unrelated event for ever. The order id should be optional at the notice boundary and an event that cannot name one of our orders should be acknowledged.
Not fixed on this branch — its job here was repairing its merge with main, and this needs its own regression test. Recorded in TODO.md; you also raised it as #discussion_r3676744931.
Generated by Claude Code
The balance-payment entry now says the redaction throws and stops all tidying behind it, and the checkout-retry entry says any unexpected error becomes the same endless retry.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b255f2972
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `SELECT ${columnsSql} | ||
| FROM payment_sessions | ||
| WHERE origin = 'current' | ||
| AND attendee_id IN (${inPlaceholders(attendeeIds)}) |
There was a problem hiding this comment.
Include migrated payments in attendee refund lookups
After the aggregate migration, historical bookings retain origin = 'legacy', but this query loads only origin = 'current'. Consequently getAttendeePaymentRefundOrNull finds no target for those bookings, so both the attendee refund page and refund-all report that no refundable payment exists even after an operator has confirmed the legacy provider evidence. Include resolved migrated sessions or promote them to the current shape when they are resolved.
Useful? React with 👍 / 👎.
| { attendee: data.attendee, references, targets }, | ||
| listingId, | ||
| ); | ||
| if (refunded.outcome !== "refunded") { |
There was a problem hiding this comment.
Queue ledger completion for pending single refunds
When a single-attendee refund is accepted by the provider as pending, refundCandidateAtProvider returns failed here and this route exits before recordAttendeeRefund. Maintenance later confirms the aggregate refund, but only the queued bulk-refund path performs the attendee ledger write, so a normal asynchronous provider refund can complete while Money permanently shows the funds as held. Persist the same retryable ledger-completion work used by bulk refunds instead of treating pending as a terminal route error.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| const currency = settings.currency; | ||
| const [emails, webhooks, sites, renewals] = await Promise.all([ | ||
| prepareRegistrationEmailDeliveries(entries, currency), | ||
| prepareRegistrationWebhookDeliveries(entries, currency), |
There was a problem hiding this comment.
Use the payment snapshot currency for delayed deliveries
If the owner changes the site currency after payment capture but before deferred completion prepares its email and webhook deliveries, this reads the new setting rather than current.payment.expected.currency. The buyer's ticket and the registration webhook then label the captured amount with the wrong currency and can scale it incorrectly when the currencies use different minor-unit divisors. Build these deliveries from the stored payment currency.
Useful? React with 👍 / 👎.
| OR (paymentSession.state IN ('completed', 'fully_refunded') | ||
| AND paymentSession.completion_state = 'completed') |
There was a problem hiding this comment.
Redact fully refunded payments without completion plans
A current payment can reach fully_refunded without ever creating a completion plan, such as when the provider refunds it before a booking is made or a changed balance is returned. Requiring completion_state = 'completed' for every fully refunded session permanently excludes that terminal shape from redaction, so its encrypted buyer and checkout details remain stored beyond the configured retention period. Treat a fully refunded session with no pending completion as eligible once its other refund and case fences pass.
Useful? React with 👍 / 👎.
| namesInMigrationResult, | ||
| } from "./legacy-payment-schema.ts"; | ||
|
|
||
| const operations = ["insert", "update"] as const; |
There was a problem hiding this comment.
Block legacy payment deletes while migration is draining
During the multi-request payment migration, an older edge-script copy can still run against the legacy tables. These triggers block inserts and updates but permit deletes, so that copy can prune a processed payment or remove a related legacy row after one migration page but before the next; the final drained-table check then sees nothing remaining and accepts the silent data loss. Fence deletes too while still allowing the migration's own claimed drain operation.
Useful? React with 👍 / 👎.
| return accounts | ||
| .filter( | ||
| ({ provider }) => payment.provider !== provider && !tried.has(provider), | ||
| ) |
There was a problem hiding this comment.
Offer the known provider when repairing legacy payments
For a migrated checkout that already identifies its provider but lacks an account and mode, this filter removes every account belonging to that provider. With the usual one-provider setup, legacyAssignmentSelections therefore returns no choices, while keep_legacy_payment is not yet offered because no legacy facts have been reviewed; the owner-facing case has no action that can resolve it. Include accounts for the known provider when the missing facts are precisely the account and mode.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| if (lookup.status !== "found") { | ||
| return pendingProviderRefund(charge, persisted); |
There was a problem hiding this comment.
Escalate missing Stripe refunds instead of polling forever
When Stripe authoritatively reports that the persisted refund ID is missing, lookup.status !== "found" takes the same path as a temporary outage and returns pending. Because the persisted refund resource is present, the shared refund logic does not open a problem case for Stripe, so the charge remains pending and maintenance requests the nonexistent ID every minute indefinitely. Distinguish missing from unavailable and record the missing refund as failed or owner-actionable.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| const ticketTokens = | ||
| runtime.processedPayment?.ticketTokens || | ||
| runtime.checkoutStage?.ticketTokens || | ||
| null; |
There was a problem hiding this comment.
Preserve consumed ticket state when migrating payments
For a completed legacy payment, an empty processed_payments.ticket_tokens means the tokens were already handed to the buyer and consumed. This || treats that meaningful empty value as absent and falls back to the stale checkout-stage copy, marking those bearer tokens ready again; after upgrade, an old provider return can therefore reveal and reuse a ticket link that had already been consumed. Trust the completed row even when its token field is empty, and consult the checkout stage only when no processed row exists.
AGENTS.md reference: AGENTS.md:L128-L137
Useful? React with 👍 / 👎.
The coverage gate, the part-answered review threads and the findings themselves are now described well enough to pick up without reading the pull request, including why the coverage work is slower than the line numbers suggest.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32e0293124
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { | ||
| throw new Error("Renewal site facts changed after payment"); | ||
| } | ||
| if (site.readOnlyFrom !== delivery.readOnlyFrom) { |
There was a problem hiding this comment.
Serialize renewals before applying the stored deadline
When two payments renew the same site before either delivery runs, both deliveries capture the same previousReadOnlyFrom and readOnlyFrom. The first delivery advances the site, then the second sees that the site already equals its stored target and skips the update while still completing its paid delivery, so one purchased renewal period is lost. Apply each renewal additively against the current deadline under a claim or transaction.
Useful? React with 👍 / 👎.
| }), | ||
| ]); | ||
| withTransaction(async (transaction) => { | ||
| await requireNoPendingAttendeePaymentCompletion(transaction, attendeeId); |
There was a problem hiding this comment.
Fence attendee deletion while refunds are unfinished
When an owner deletes an attendee after a queued bulk refund becomes pending, this guard passes because it checks only completion_state = 'pending', and the following batch detaches the refunding payment. If the provider later confirms the refund, bulk-refund maintenance calls finishQueuedBulkRefund with a null attendee and throws, leaving the attendee ledger uncorrected and the repair case stuck. Refuse deletion while the attendee has unfinished refund work as well.
Useful? React with 👍 / 👎.
| const TERMINAL_HANDLERS: Record<PaymentSession["state"], TerminalHandler> = { | ||
| completed: completedOutcome, | ||
| created: unfinishedOutcome, | ||
| failed: conflictOutcome, |
There was a problem hiding this comment.
Keep failed checkout replays on the cancellation path
For a refused or cancelled SumUp checkout, the first reconciliation stores state = 'failed' and returns ignore, which lets the redirect render the cancellation page. On a reload, this terminal mapping instead returns conflict; the redirect then tells the buyer that the payment needs review even though no payment case exists. Return the same cancellation-compatible outcome for terminal failed checkouts.
Useful? React with 👍 / 👎.
| if (page.length < MIGRATION_PAGE_SIZE) { | ||
| return { complete: true, count: count + 1 }; |
There was a problem hiding this comment.
Check raw migration rows before declaring the page complete
When the 25 selected legacy IDs include aliases that mergeLegacyPaymentRows combines into one payment, page.length is below 25 even though the raw source page was full and more rows remain. This branch falsely declares the session source complete, proceeds to the drained-source assertion, and turns an expected multi-request upgrade into a hard migration failure for each affected chunk. Base completion on the raw source count or re-read the source after draining.
Useful? React with 👍 / 👎.
| currency: settings.currency.toUpperCase() as Currency, | ||
| description: `Tickets (${intent.items.length} listing(s))`, | ||
| const created = await client.checkouts.create({ | ||
| amount: Number(toMajorUnits(expected.amount)), |
There was a problem hiding this comment.
Convert SumUp amounts with the stored currency
When a durable SumUp checkout is retried after the owner changes the site currency, expected.amount and expected.currency still describe the original payment, but toMajorUnits uses the live settings.currency divisor. Switching from GBP to a zero-decimal currency can therefore send 1,000 minor units as a GBP 1,000 charge instead of GBP 10; the response parser uses the same live divisor, so its expected-money check can still pass. Convert both requests and responses using their explicit stored currency.
Useful? React with 👍 / 👎.
| } | ||
| const checkoutId = | ||
| requested.kind === "sumup_checkout" ? requested.id : requested.parentId; | ||
| const result = await sumupApi.retrieveCheckoutById(checkoutId); |
There was a problem hiding this comment.
Check unsigned SumUp IDs before calling the provider
SumUp webhooks are unsigned, and the webhook route treats requests without another provider's signature header as SumUp. Any syntactically valid payload therefore reaches this fetch before paymentForCheckout checks whether the checkout reference belongs to a local payment, so arbitrary spoofed IDs each consume an edge subrequest and a SumUp API call. Look up the provider resource locally and acknowledge unknown IDs before fetching SumUp.
Useful? React with 👍 / 👎.
A short-looking migration page fails the whole upgrade rather than skipping a tail, and a SumUp retry after a currency change can send a hundred times the money with the check still passing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f7b8b87cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| attendee: { id: attendeeId }, | ||
| listingId: completion.result.listingId, | ||
| success: true, | ||
| ticketTokens: completion.result.ticketTokens, |
There was a problem hiding this comment.
Honor consumed state when replaying pending completions
When a redirect has consumed payment.ticketTokens but completion effects are still pending, terminalPaymentOutcome rebuilds a fulfilled result through this function, and this line returns the immutable token copy in completion.result without checking ticketState. A replay during that window therefore reveals the bearer ticket URL again—indefinitely if a delivery remains stuck. This is fresh evidence beyond the earlier consume-condition finding: extending consumption to fulfilled only clears payment_sessions.ticket_tokens, not this second completion copy, so rebuilt results must also honor the consumed state.
Useful? React with 👍 / 👎.
| const targets = payments.map((payment): PaymentRefundTarget => { | ||
| const stored = chargesByPaymentId.get(payment.id) ?? []; | ||
| return { charges: currentPaymentCharges(payment, stored), payment }; |
There was a problem hiding this comment.
Exclude charge-less aggregates from refund targets
When an attendee owns both a charged session and an attached zero-total session with no charge rows—possible after an attendee merge, which repoints every aggregate—this creates a refund target with charges: []. The candidate still qualifies because of the charged reference, but refundPaymentTargets later calls refundCharges on the empty target and throws. If the charged target comes first, the provider refund has already moved money before the error causes the attendee-ledger write to be skipped; if the empty target comes first, the valid refund is blocked entirely. Omit aggregates without charges from the target list.
Useful? React with 👍 / 👎.
| pending_refund_idempotency_key = CASE WHEN ? IN ('pending', 'failed') | ||
| THEN pending_refund_idempotency_key ELSE NULL END, | ||
| pending_refund_key_index = CASE WHEN ? IN ('pending', 'failed') | ||
| THEN pending_refund_key_index ELSE NULL END, |
There was a problem hiding this comment.
Rotate the key after a terminal failed refund
When Stripe or Square returns a definitive failed refund resource, retaining the idempotency key makes every automatic retry and the owner's later refund_remaining decision submit the same failed operation again. These providers bind the result to that key, so the request cannot become a new refund; the case reaches needs_action, but the offered repair action remains permanently ineffective unless the owner refunds manually and falsely bypasses it with confirmation. Keep the key for uncertain transport outcomes, but clear or replace it after a terminal provider failure so an explicit retry can create a new operation.
Useful? React with 👍 / 👎.
A ticket link kept in two places so only one is marked used, a failed refund that keeps the key that made it fail, and an empty payment that joins the refund list and stops a real refund.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 158777521a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| paymentSessionAttendeeChangeStatement( | ||
| { args: [sourceId], sql: "?" }, | ||
| targetId, | ||
| ), |
There was a problem hiding this comment.
Fence attendee merges while paid delivery is pending
When the source attendee has a pending paid-site delivery, this updates only payment_sessions.attendee_id and then deletes the source attendee. The already-stored payment_completion_deliveries row still contains the source ID, and applyPaidSiteAssignment uses that ID when reserving the site, so completion can assign the purchased site to an attendee that no longer exists. Block the merge until completion finishes, or repoint the stored delivery and any reserved site in the same transaction.
Useful? React with 👍 / 👎.
| ); | ||
| const next = pending[0]; | ||
| if (next === undefined) return true; | ||
| await deliverPaymentCompletionData(current, next.id, next.data); |
There was a problem hiding this comment.
Retire permanently rejected completion deliveries
If a prepared email or registration webhook receives a permanent refusal, such as an unsafe URL or HTTP 400, this await throws before the delivery is marked complete or actionable. The same oldest delivery is therefore retried indefinitely, the buyer's payment never finishes its remaining effects, and the one-row maintenance query repeatedly puts it ahead of later due payments. Persist a terminal failure or owner-actionable case instead of leaving the row pending forever.
Useful? React with 👍 / 👎.
| config: { | ||
| fromAddress: prepared.config.fromAddress, | ||
| provider: prepared.config.provider, | ||
| }, |
There was a problem hiding this comment.
Revalidate the saved business recipient before sending
If the owner changes the business email after a paid completion prepares its messages but before maintenance sends them, the stored admin notification still targets the old address. Only the provider and from-address are saved and later compared, so the notification—including the buyer's personal details—is delivered to the former mailbox even though it is no longer configured. Store and validate the business recipient with the other email configuration facts.
Useful? React with 👍 / 👎.
| CASE WHEN attendee_id IS NULL THEN NULL ELSE ( | ||
| SELECT booking.listing_id FROM listing_attendees AS booking | ||
| WHERE booking.attendee_id = processedPayment.attendee_id | ||
| ORDER BY booking.quantity > 0 DESC, booking.id LIMIT 1 | ||
| ) END AS listingId, |
There was a problem hiding this comment.
Migrate completed payments whose booking row was removed
When a historical processed_payments row still names its attendee but that attendee's listing_attendees rows were deleted, this scalar subquery returns NULL. LegacyProcessedPaymentSchema rejects the resulting attendee-without-listing shape, so the payment aggregate migration aborts and every later request retries the same failed upgrade. Preserve such completed payment evidence without requiring a live booking row, or turn it into an owner-actionable legacy case.
Useful? React with 👍 / 👎.
| ): Promise<ProviderRead> => { | ||
| if (initialRead !== null) return initialRead; | ||
| return readProviderOrInvalid(context.payment, context.requested, async () => { | ||
| const account = await resolvePaymentAccount(context.providerType); |
There was a problem hiding this comment.
Release claimed payments when account resolution fails
If credentials were removed or the provider account lookup is temporarily unavailable, this call throws after reconcilePayment has already claimed the payment. No retry time or case is recorded and the five-minute lease is left in place; with a lasting configuration problem, maintenance repeatedly reclaims and fails the paid checkout without ever completing the booking or showing the owner an actionable case. Persist a retry for transient lookup failures and a needs-action case for configuration failures.
AGENTS.md reference: AGENTS.md:L87-L87
Useful? React with 👍 / 👎.
| `Listing ${item.e} was not loaded for payment completion`, | ||
| ), | ||
| })); | ||
| return committedEntries( |
There was a problem hiding this comment.
Freeze booking facts before delayed completion work
If an owner edits the committed booking or listing after the payment creates it but before its delayed effects run, this rebuilds completion entries from the live rows rather than the facts committed with the payment. Changing quantity can therefore make site assignment hand out more sites than were purchased, while emails, webhooks, and activity can describe the edited booking instead of the paid one. Persist the delivery-facing booking snapshot when the booking commits and resume from that snapshot.
Useful? React with 👍 / 👎.
| if (!(event.status in REFUND_STATUS)) { | ||
| throw new Error(`Unsupported SumUp refund status: ${event.status}`); |
There was a problem hiding this comment.
Escalate unsupported SumUp refund states
When SumUp returns a refund event in an allowed but unmapped state such as SCHEDULED, this throws an ordinary Error, which readSumupTransport converts to unavailable. The aggregate then treats the permanent response shape as a temporary outage and asks about the same refund every minute forever without opening a case for the owner. Classify an unsupported state as invalid or needs-action rather than transport unavailability.
AGENTS.md reference: AGENTS.md:L138-L146
Useful? React with 👍 / 👎.
The unmapped SumUp refund state throws in the boundary file, and the payment with no booking is refused by the migration as well as the copy.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cf6929b42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await runPaymentDecisionMaintenance(context); | ||
| await runPaymentReconciliationMaintenance(context); |
There was a problem hiding this comment.
Continue reconciliation after one decision retry
When a due owner decision hits a provider error, executeAcceptedDecision persists it as retrying and then rethrows; this await therefore aborts runPaymentMaintenance before ordinary reconciliation starts. If that decision keeps failing, every scheduled run can stop before processing all other checkouts, completions, and refunds, so treat the persisted retry as this item's outcome and continue with the remaining work.
AGENTS.md reference: AGENTS.md:L87-L87
Useful? React with 👍 / 👎.
| if (context.payment.completionState === "pending") { | ||
| return fulfilStoredCompletion(context.payment, claim, fulfil); | ||
| } | ||
| if (context.payment.state === "refunding") { | ||
| return retryRefund(context.payment, claim); |
There was a problem hiding this comment.
Run queued refunds before pending completion
When refund-all queues a payment whose paid completion is still pending, the queue changes its state to refunding without clearing completionState. This ordering resumes completion first; its final write attempts the forbidden refunding -> completed transition and throws, so maintenance retries that completion forever and never reaches retryRefund, leaving the requested refund stuck.
Useful? React with 👍 / 👎.
| sql: `SELECT paymentSessionId FROM (${sources.join(" UNION ")}) | ||
| ORDER BY paymentSessionId LIMIT ?`, |
There was a problem hiding this comment.
Group both SumUp identifiers before paging
When a SumUp checkout's local reference and provider checkout ID sort onto different 25-ID pages, the first page expands to and drains their shared sumup_checkouts row but reads only the legacy session rows for one identifier. The later page then copies the remaining session rows as a second aggregate, separating the provider alias from payment or booking evidence; page by complete SumUp groups rather than independent IDs.
Useful? React with 👍 / 👎.
| export const redactBookingIntent = (intent: BookingIntent): BookingIntent => ({ | ||
| address: "", | ||
| date: null, | ||
| email: "", | ||
| items: intent.items, |
There was a problem hiding this comment.
Preserve balance identity while redacting intents
When an old completed balance payment becomes eligible for redaction, this reconstruction drops balanceAttendeeId. Its stored completion still has facts.flow = "balance", so PaymentCompletionSchema rejects the redacted completion while sealing it; because the page prepares every statement with one Promise.all, that payment blocks its own retention cleanup and every later payment-history cleanup on each run.
Useful? React with 👍 / 👎.
| const localPaymentId = (request: Request): string => | ||
| getSearchParam(request, "payment_id"); |
There was a problem hiding this comment.
Treat SumUp session_id as the local payment ID
SumUp creates its return URL as ?session_id=<localPaymentId>, but only payment_id enters the local lookup path here. If the owner switches the active provider while that checkout is open, the return is interpreted as the new provider's session ID, the existing SumUp aggregate is not found, and the buyer cannot reach the completed ticket; recognize SumUp's session_id local ID independently of the current setting.
Useful? React with 👍 / 👎.
| email: intent.email, | ||
| name: intent.name, | ||
| paymentId: session.paymentReference, | ||
| paymentId: "", |
There was a problem hiding this comment.
Show aggregate references for new paid bookings
Every booking created through the new paid path now stores an empty payment_id, while the attendee detail (src/ui/templates/admin/attendees.tsx) and transaction-ID CSV column still read that field. Consequently all new paid bookings lose their visible provider reference on those operator surfaces even though the aggregate retains the charge evidence; migrate those consumers to the aggregate or populate a suitable displayed reference.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| export const configuredPaymentAccounts = (): Promise<PaymentAccount[]> => | ||
| Promise.all(configuredPaymentProviders().map(resolvePaymentAccount)); |
There was a problem hiding this comment.
Keep working repair choices when one provider is down
When several providers are configured and one account lookup fails—for example Stripe's account endpoint is temporarily unavailable—this Promise.all rejects the entire legacy payment-case page. The owner then cannot use otherwise valid Square or SumUp choices to repair the payment, so resolve account choices independently and make only the unavailable provider unusable.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| if (remaining.size === 0) return { payments: found }; | ||
| if (page === SQUARE_PAYMENT_PAGE_LIMIT) return { issue: "unavailable" }; | ||
| const result = await load({ |
There was a problem hiding this comment.
Fetch Square's documented tender payments directly
Once a Square location has more than 800 payments, a newly completed tender can fall beyond these eight ascending 100-payment pages. Although the order names the exact tender ID and the client exposes payments.get, reconciliation reports the payment as unavailable, so every subsequent Square checkout can remain unfulfilled as the location grows; retrieve the order's documented payment IDs directly instead of scanning the account history.
Useful? React with 👍 / 👎.
| actions: placeholderCompletionActions, | ||
| criticalEffects: ["provider_refund"], | ||
| effects: PlaceholderRefundEffectSchema.options, |
There was a problem hiding this comment.
Record placeholder money before waiting for its refund
When a captured payment cannot be booked and its provider refund remains pending or needs owner action, provider_refund runs first and is the only critical effect, so completion pauses before payment_ledger and pending_note. A refund that stays unresolved therefore leaves Money missing the captured payment and gives the operator no attendee note explaining it; persist those local facts before starting or waiting for the provider refund.
Useful? React with 👍 / 👎.
| const payment = requireCurrentPayment(context); | ||
| await confirmChargesFullyRefunded(payment.id, decision.charges); | ||
| return "completed"; |
There was a problem hiding this comment.
Mark confirmed refunds fully refunded
When the owner chooses confirm_fully_refunded, this updates only the charge rows and then resolves the decision and case. The payment session itself remains in its previous needs_action or completed state, so future callbacks can still report that the payment needs review and a needs_action aggregate never becomes eligible for redaction; advance the session to fully_refunded as part of the confirmation.
Useful? React with 👍 / 👎.
This branch takes the durable payment work from #1905 and makes it a base that
chunks can safely be split off, one at a time, the way #1853 was split.
It is not ready to merge as it stands. It is here so the work is visible, so CI
runs it on every push, and so the chunks that come off it start from something
that builds and passes.
Why the branch exists
The payment work in #1905 was checked with
deno check src/edge.ts, which readsthe source only. The full check also reads the tests, and it found 67 errors
there: production code had been renamed and moved without the tests following.
Running the tests then found 216 failures. Neither was visible before.
Where it is now
Passing: type checking, linting, the copy checks, the duplicate-code check at
its 0% threshold, the whole test suite, and every Cucumber story. The branch
merges cleanly into main.
Failing: the coverage gate. It stood at 1,011 uncovered lines when this started
and is now around 700, across roughly 70 files.
Faults in the payment work, found and fixed
These are the ones that would have reached people using the site.
A closed listing told the operator the price had changed. Every booking
refused for a reason other than a missing listing was filed as a price change,
and no code could produce the "registration closed" reason any more. The reason
is back and is chosen from what actually happened.
A paid booking was thrown away when one written answer lost its id. A
stricter shape for the answers carried through checkout meant one bad answer
failed the whole booking, so the callback quietly did nothing: money taken, no
booking, no refund, nothing written down.
A payment key that stopped working crashed the checkout. Looking up the
account happened before anything could catch a failure, so a rotated or revoked
key met the buyer with a crash instead of a message.
Every checkout failed to start. Creating one demanded the full settled
shape back, including the amount and currency, which a checkout nobody has paid
yet does not carry. Creation now checks the two things it uses: the id, and the
link to send the buyer to.
A refund with no provider set up blamed the payment. It said the payment
may already have been refunded, sending the operator looking for a refund that
never happened. It now says payments are not set up.
An upgrade could strand its own lock. Upgrading spent 58 database calls
where a request is allowed 50. It ran out before it could write down what it
had done, and releasing the lock needs a call of its own, so the lock stayed
held and every following request was refused for two minutes — then did the
same again. The upgrade now takes 31 calls: the old payment tables are read
once in a batch rather than one table at a time.
What is tested now that was not
Whole behaviours, not just lines:
still going, the provider cannot be reached, the payment was refunded, or the
provider disagrees about the total.
email, refusing to send it once the email settings have changed, and refusing
to email about a site that was never handed out.
reuses it rather than taking another.
every way that can be refused — the site was removed, it is not the site that
was paid for, its deadline moved, the host would not take the change, or the
site goes mid-change.
reports a transaction that no longer matches what we captured.
and the payment has moved under it.
Tidying done along the way
Eight places where the duplicate-code check pointed at two things that should
be one, merged rather than worked around — including one helper that replaced
seven copies of the same webhook setup. Two fixtures nothing called were
deleted, and a guard that could never run was restructured so the case it was
meant to catch is now reachable and tested.
What is left, and where it is written down
TODO.mdcarries the outstanding work with a starting point for each:onto the new payment tables. Old payments cannot be refunded from an
attendee's page; money sent back is not written into the books; a paused
refund can jam the queue; and five more. The owner repair path blocks several
of the others, so it comes first.
clearing them is
test/shared/renewal-failures.test.ts.decision-attempts.tsreports acheck as unrun that instrumenting proves does run. It is written up with the
evidence, including a wrong turn taken while chasing it.
decision-attempts.tsrestates in TypeScript a condition the SQL alreadyexpresses, and only runs to explain that condition's failure. Whether it
should exist is a decision about money-path code, not something to settle
with more tests.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
Generated by Claude Code