Finish and recover paid checkouts safely - #1853
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces placeholder bookings with staged checkout records and zero-quantity attendees. Paid sessions activate staged attendees atomically; failed, cancelled, expired, or mismatched sessions close and refund staged checkouts. Provider contracts, webhook retries, migrations, pruning, and payment-flow tests are updated. ChangesStaged checkout and activation
Provider and route integration
Test infrastructure
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant Customer
participant TicketPayment
participant StagedCheckout
participant Provider
participant Database
Customer->>TicketPayment: submit paid checkout
TicketPayment->>StagedCheckout: createAndHandlePaidCheckout
StagedCheckout->>Provider: createCheckoutSession
Provider-->>StagedCheckout: checkout identifiers and URL
StagedCheckout->>Database: create pending staged attendee and checkout stage
StagedCheckout-->>Customer: checkout URL
sequenceDiagram
participant Provider
participant Webhooks
participant PaymentProcessing
participant Attendees
participant Refunds
Provider->>Webhooks: resolve webhook session
Webhooks->>Webhooks: classify completed or expired event
Webhooks->>PaymentProcessing: process paid session
PaymentProcessing->>Attendees: activate staged attendee
alt activation succeeds
Attendees-->>Webhooks: success
else activation or validation fails
PaymentProcessing->>Refunds: refund staged booking
Refunds-->>Webhooks: terminal failure or retry
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
test/lib/server-webhooks/refund-helper-functions.test.ts (1)
150-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStage this checkout before exercising the activation failure.
The paid-session flow loads its checkout stage before activation. Without staging
cs_create_boom, this test cannot reach the mocked rejection or refund assertion. CallstageStripeCallback("cs_create_boom")before the success request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-webhooks/refund-helper-functions.test.ts` around lines 150 - 180, Update the test “an unexpected uncommitted activation error refunds” to call stageStripeCallback("cs_create_boom") after configuring the checkout session and before handleRequest exercises the success flow, ensuring the staged checkout reaches the mocked activateStagedAttendee failure and refund assertion.src/features/api/payment-processing/create.ts (1)
249-259: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail when a priced booking line has no paid amount.
paidByIntentItem.get(...)is expected to resolve, butundefinedsilently omitspricePaid, allowing the activated booking and its ledger plan to disagree. Use a throwing lookup before activation.As per coding guidelines, expected lookup values must throw when absent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/api/payment-processing/create.ts` around lines 249 - 259, Update the booking construction inside bookingsForOrder to use a throwing lookup for each pricingIntent item instead of allowing paidByIntentItem.get(...) to return undefined. Always pass the resolved pricePaid into the booking, so activation fails before creating inconsistent booking or ledger state when the amount is missing.Source: Coding guidelines
test/lib/server-payments/success.test.ts (1)
220-238: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUpdate the stale placeholder comments.
Lines 220-236 still claim that a quantity-zero placeholder and system note are retained, while Lines 237-238 assert the opposite. Describe the current staged-refund behavior instead. As per coding guidelines, comments must explain current code and not historical implementations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-payments/success.test.ts` around lines 220 - 238, Update the comments surrounding the signed-by-us late-buyer refund scenario in the test to describe the current staged-refund behavior reflected by expectNoRefundPlaceholder and expectRefundedWithoutAttendee. Remove claims that a quantity-zero placeholder, system note, or retained booking is created, while preserving the existing assertions and response expectations.Source: Coding guidelines
src/shared/payment-helpers.ts (1)
414-451: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire providers to supply the hosted-checkout identifier explicitly.
Defaulting
providerCheckoutIdtosessionIdmasks an omitted Square/SumUp mapping and later closes the wrong provider resource. Make this field required inreadResultand lettoCheckoutResultreject an absent value.As per coding guidelines, missing required external fields must fail at the boundary and must not be defaulted.
Proposed contract fix
export const toCheckoutResult = ( sessionId: string | undefined, url: string | undefined | null, label: LogCategory, - providerCheckoutId = sessionId, + providerCheckoutId: string | undefined, ): CheckoutSessionResult => {readResult: (result: Result) => { id: string | undefined; - providerId?: string | undefined; + providerId: string | undefined; url: string | undefined | null; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/payment-helpers.ts` around lines 414 - 451, Require an explicit providerCheckoutId in toCheckoutResult instead of defaulting it to sessionId, and make the corresponding providerId field required in makeCreateCheckoutSession’s readResult contract. Update all provider mappings to supply the hosted-checkout identifier explicitly, while preserving rejection when it is absent.Source: Coding guidelines
test/lib/server-webhooks/multi-ticket-refunds.test.ts (1)
87-93: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUpdate refund-test descriptions for the staged-removal model.
These comments still describe the removed quantity-zero placeholder behavior, while the changed assertions prove that staged attendees are deleted after refund.
test/lib/server-webhooks/multi-ticket-refunds.test.ts#L87-L93: describe staged-attendee removal and one refund.test/lib/server-webhooks/multi-ticket-refunds.test.ts#L133-L135: describe removal from both listings.test/lib/server-webhooks/multi-ticket-refunds.test.ts#L170-L176: replace placeholder retention with staged-attendee removal.test/lib/server-webhooks/multi-ticket-refunds.test.ts#L207-L213: replace placeholder retention with staged-attendee removal.test/lib/server-webhooks/single-ticket-refunds.test.ts#L64-L70: describe staged-attendee removal after refund.test/lib/server-webhooks/single-ticket-refunds.test.ts#L131-L137: describe staged-attendee removal in the redirect path.As per coding guidelines, comments must explain the current code and not historical implementations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-webhooks/multi-ticket-refunds.test.ts` around lines 87 - 93, Update the comments describing refund assertions to reflect the current staged-attendee removal behavior rather than quantity-zero placeholder retention: in test/lib/server-webhooks/multi-ticket-refunds.test.ts ranges 87-93, 133-135, 170-176, and 207-213, describe removal from the relevant listing(s) and the single refund; in test/lib/server-webhooks/single-ticket-refunds.test.ts ranges 64-70 and 131-137, describe staged-attendee removal after refund, including the redirect path.Source: Coding guidelines
test/lib/server-webhooks/refund-skip-conditions.test.ts (1)
128-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest the scenario named by this test.
This uses unsigned
webhookMetaand asserts an ignored, non-refunded webhook. It does not exercise a signed checkout with a missing listing. Split the unsigned-ignore case or construct the signed case and assert its refund/removal outcome.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-webhooks/refund-skip-conditions.test.ts` around lines 128 - 162, The test currently covers only the unsigned webhook path, not the signed checkout with a missing listing. Update the test around checkoutSessionEvent to construct valid signed metadata for the multi-ticket payload, then assert the expected refund and attendee-removal outcome; move the existing unsigned ignored/no-refund assertions into a separate test if they should remain covered.test/lib/server-webhooks/modifier-refunds.test.ts (1)
51-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the obsolete quantity-zero placeholder documentation.
The assertions now require staged attendee deletion, but nearby comments still describe retaining a placeholder.
test/lib/server-webhooks/modifier-refunds.test.ts#L51-L55: remove or rewrite the preceding placeholder/failure-state comments.test/lib/server-webhooks/modifier-refunds.test.ts#L89-L93: update the retained-placeholder description to staged-attendee deletion.test/lib/server-webhooks/modifier-refunds.test.ts#L130-L137: describe deletion and the resulting absence of contact history.test/lib/server-webhooks/multi-ticket-booking.test.ts#L137-L138: replace the retained-placeholder comment with the no-attendee outcome.test/lib/webhook-price-signature/stored-refund-and-ignore.test.ts#L61-L72: remove references to an attendee netting to zero and a quantity-zero projected price.As per coding guidelines, “Do not leave historical comments describing replaced or previous implementations.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-webhooks/modifier-refunds.test.ts` around lines 51 - 55, Remove or rewrite the obsolete quantity-zero and retained-placeholder comments at test/lib/server-webhooks/modifier-refunds.test.ts:51-55, 89-93, and 130-137 to describe staged attendee deletion and the resulting absence of contact history; update test/lib/server-webhooks/multi-ticket-booking.test.ts:137-138 to describe the no-attendee outcome; and remove references to netting an attendee to zero or a quantity-zero projected price in test/lib/webhook-price-signature/stored-refund-and-ignore.test.ts:61-72. Keep comments aligned with the current assertions and implementation, without describing historical behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/api/webhooks.ts`:
- Around line 435-447: Update the expired/failed branch in the webhook handler
to inspect the result from tryCloseAndPurgeCheckoutStageBySession instead of
always returning a closed acknowledgement. Handle paid by freshly retrieving the
session and returning a successful response only when its paid state is
readable; return 503 when closure fails or the paid state cannot yet be read so
the webhook is retried. Handle error results with an explicit recovery path
rather than catching and continuing.
In `@src/shared/booking-lines.ts`:
- Around line 131-143: Update checkoutBookingLines so the listingById lookup
fails explicitly when no listing exists, replacing the unchecked assertion with
an immediate error that includes item.listingId. Preserve the existing line
construction for items with loaded listings.
In `@src/shared/db/checkout-stages.ts`:
- Around line 288-296: Remove the parallel checkout-stage API object and its
aliases from checkoutStagesApi, keeping the existing direct operation exports as
the single public surface. Migrate all callers of checkoutStagesApi.beginRefund,
cleanupLimit, finalizeRefund, find, loadByPaymentSession, purgePending, and
selectOldPending to the corresponding direct exports, then remove the
compatibility object.
- Around line 132-136: Update the read queries in the checkout-stage data access
flow, including the queries near the visible SELECT and the locations noted at
lines 159-164 and 205-207, to use `FROM checkout_stages AS checkout_stage`.
Qualify every selected and filtered checkout_stages column with the
`checkout_stage` alias consistently.
In `@src/shared/square.ts`:
- Around line 526-535: Update the response handling in the Square payment-link
flow around id, orderId, and url so missing required fields fail at the boundary
by throwing an explicit error instead of logging and returning null. Preserve
the successful return of { id, orderId, url } when all fields are present.
In `@src/shared/stripe.ts`:
- Line 458: Update the expires_at calculation in the Stripe session
configuration to use a buffer beyond the 30-minute minimum, such as 31 minutes,
instead of exactly 30 minutes. Keep the existing nowSeconds-based expiration
calculation unchanged otherwise.
In `@test/features/api/payment-processing/recovery.test.ts`:
- Around line 1-26: Remove the jscpd suppression directives around the import
blocks in test/features/api/payment-processing/recovery.test.ts (lines 1-26),
staged-refund-errors.test.ts (lines 1-25), staged-refunds.test.ts (lines 2-25),
and staged-runtime.test.ts (lines 1-18). Extract any genuine duplicated setup
into shared test utilities as needed, without restructuring code solely to evade
duplicate detection.
In `@test/lib/db/attendees/create-attendee-atomic.test.ts`:
- Around line 131-132: Update the test’s result handling so attendee creation
failure causes an immediate assertion failure instead of returning early. In the
success path, retain the existing getAttendeeRaw assertion for the created
attendee.
In `@test/lib/db/attendees/select.test.ts`:
- Line 14: Update the attendee select tests to stop importing or interpolating
ordinaryAttendeeCondition when constructing expected SQL; assert the literal NOT
EXISTS predicate directly in the affected exact-string cases. Add a focused unit
test for ordinaryAttendeeCondition itself covering its generated clause.
In `@test/lib/server-payments/cancel-race.test.ts`:
- Around line 1-19: Remove the jscpd:ignore-start and jscpd:ignore-end comments
from cancel-race.test.ts, then extract the genuinely repeated test setup into an
appropriate shared test helper and reuse it from this test so duplicate-code
detection passes without suppression.
In `@test/lib/server-payments/success.test.ts`:
- Around line 103-134: Update the test around handleRequest and the
closeCheckout stub to assert that the provider checkout is closed. Verify
closeCheckout is called exactly once with the expected failed session identifier
and provider checkout arguments, while preserving the existing response-status
and attendee-purge assertions.
In `@test/lib/server-reservation/deposit-basics.test.ts`:
- Around line 159-164: Extend the invalid-reservation test after the
attendee-count assertion to verify the refund mock’s invocation through
refund.calls and confirm the payment record reached its terminal refunded state.
Use the existing refund and payment-record symbols in the test, preserving the
current response and attendee-removal assertions.
In `@test/lib/server-reservation/edge-cases.test.ts`:
- Around line 112-121: Update stale test terminology to describe the current
staged-checkout removal behavior: rename the sold-out add-on test in
test/lib/server-reservation/edge-cases.test.ts:112-121 and mismatched zero-price
add-on test in test/lib/server-reservation/promo-addons.test.ts:132-140 to
mention refund and removal; replace retained-placeholder comments in
test/lib/server-webhooks/already-processed-rollback.test.ts:114-120,
test/lib/server-webhooks/can-pay-more-multi-ticket.test.ts:118-122 and :193-199,
and test/lib/server-webhooks/can-pay-more-single-ticket.test.ts:83-89 and
:119-125 with comments describing removal, including refund where applicable.
In `@test/lib/server-webhooks/custom-questions-multi.test.ts`:
- Around line 51-56: Replace the native array filter in realBookings in
test/lib/server-webhooks/custom-questions-multi.test.ts:51-56 with the
documented curried filter helper from `#fp`, adding the required documented FP
import. Apply the same `#fp` import and curried filtering style at
test/lib/server-webhooks/custom-questions-single.test.ts:39-41; preserve the
existing quantity > 0 behavior.
In `@test/lib/server-webhooks/session-resolution.test.ts`:
- Around line 128-155: Update the test “an expiry close failure keeps the stage
for scheduled retry” to load the checkout stage identified by “cs_expiry_retry”
after the webhook is ignored, and assert that it remains in its expected
retryable state. Keep the existing attendee and close-call assertions, but use
the stage lookup to detect accidental deletion.
In `@test/lib/webhook-price-signature/post-commit-recovery.test.ts`:
- Around line 281-282: Add direct assertions in the recovery test after the
repeated expectStoredRefund calls to verify that both the attendee and checkout
stage are absent. Use the test’s existing stage/attendee lookup helpers or
observable storage APIs, ensuring cleanup failures cause the test to fail while
preserving the replay and stored-failure assertions.
In `@test/shared/db/attendees/activate-staged.helpers.ts`:
- Around line 77-85: Update the exported storedActivationRows helper with a
named type describing its selected row shape, and explicitly declare its return
type as Promise of an array of that type. Keep the existing query and ordering
unchanged.
In `@test/shared/db/prune/checkout-stages.test.ts`:
- Around line 151-171: Make the test data ordering deterministic in the
“processes exactly the fixed cleanup bound” test by assigning every added stage
the same fixed createdAt value or inserting the stages sequentially instead of
using unordered Promise.all writes. Keep the cleanup-limit assertions unchanged
and ensure the expected remaining ID matches the deterministically newest stage.
In `@test/shared/square-checkout-close.test.ts`:
- Around line 157-165: Add a separate test alongside the existing wrong-order
case for closePaymentLink, using a provider response with id "other" and
cancelledOrderId "order"; assert that squareApi.closePaymentLink("link",
"order") rejects with the same wrong payment-link or order error, covering
independent validation of the returned payment-link ID.
In `@test/test-utils/settings.ts`:
- Around line 222-224: Require a valid string payment reference at both staging
boundaries: in test/test-utils/settings.ts lines 222-224, validate
payment_intent before invoking stagePaymentCallback instead of defaulting it to
an empty string; in test/test-utils/webhooks.ts lines 308-310, reject or skip
staging when session.paymentIntent is null. Preserve staging only for records
with an actual external payment identifier.
In `@test/test-utils/staged-payments.ts`:
- Around line 72-76: Update the listing lookup in the staged-payment
construction to fail when listingById lacks the referenced item.e entry, instead
of supplying fabricated customisable_days, duration_days, and listing_type
defaults. Preserve the existing listing data path for found listings and make
the boundary failure explicit before checkout metadata is created.
- Around line 107-112: Update the fallback pending checkout stage construction
in pendingCheckoutStageInsert to use fields.provider for payment provider and
fields.providerCheckoutId for providerCheckoutId, while retaining
fields.sessionId for paymentSessionId.
In `@test/test-utils/webhooks.ts`:
- Around line 160-171: Update the stale comment inside expectNoRefundPlaceholder
to describe that the helper asserts no quantity-zero placeholder records remain;
remove the outdated reference to an exactly-one-placeholder invariant while
leaving the filtering and assertion unchanged.
---
Outside diff comments:
In `@src/features/api/payment-processing/create.ts`:
- Around line 249-259: Update the booking construction inside bookingsForOrder
to use a throwing lookup for each pricingIntent item instead of allowing
paidByIntentItem.get(...) to return undefined. Always pass the resolved
pricePaid into the booking, so activation fails before creating inconsistent
booking or ledger state when the amount is missing.
In `@src/shared/payment-helpers.ts`:
- Around line 414-451: Require an explicit providerCheckoutId in
toCheckoutResult instead of defaulting it to sessionId, and make the
corresponding providerId field required in makeCreateCheckoutSession’s
readResult contract. Update all provider mappings to supply the hosted-checkout
identifier explicitly, while preserving rejection when it is absent.
In `@test/lib/server-payments/success.test.ts`:
- Around line 220-238: Update the comments surrounding the signed-by-us
late-buyer refund scenario in the test to describe the current staged-refund
behavior reflected by expectNoRefundPlaceholder and
expectRefundedWithoutAttendee. Remove claims that a quantity-zero placeholder,
system note, or retained booking is created, while preserving the existing
assertions and response expectations.
In `@test/lib/server-webhooks/modifier-refunds.test.ts`:
- Around line 51-55: Remove or rewrite the obsolete quantity-zero and
retained-placeholder comments at
test/lib/server-webhooks/modifier-refunds.test.ts:51-55, 89-93, and 130-137 to
describe staged attendee deletion and the resulting absence of contact history;
update test/lib/server-webhooks/multi-ticket-booking.test.ts:137-138 to describe
the no-attendee outcome; and remove references to netting an attendee to zero or
a quantity-zero projected price in
test/lib/webhook-price-signature/stored-refund-and-ignore.test.ts:61-72. Keep
comments aligned with the current assertions and implementation, without
describing historical behavior.
In `@test/lib/server-webhooks/multi-ticket-refunds.test.ts`:
- Around line 87-93: Update the comments describing refund assertions to reflect
the current staged-attendee removal behavior rather than quantity-zero
placeholder retention: in test/lib/server-webhooks/multi-ticket-refunds.test.ts
ranges 87-93, 133-135, 170-176, and 207-213, describe removal from the relevant
listing(s) and the single refund; in
test/lib/server-webhooks/single-ticket-refunds.test.ts ranges 64-70 and 131-137,
describe staged-attendee removal after refund, including the redirect path.
In `@test/lib/server-webhooks/refund-helper-functions.test.ts`:
- Around line 150-180: Update the test “an unexpected uncommitted activation
error refunds” to call stageStripeCallback("cs_create_boom") after configuring
the checkout session and before handleRequest exercises the success flow,
ensuring the staged checkout reaches the mocked activateStagedAttendee failure
and refund assertion.
In `@test/lib/server-webhooks/refund-skip-conditions.test.ts`:
- Around line 128-162: The test currently covers only the unsigned webhook path,
not the signed checkout with a missing listing. Update the test around
checkoutSessionEvent to construct valid signed metadata for the multi-ticket
payload, then assert the expected refund and attendee-removal outcome; move the
existing unsigned ignored/no-refund assertions into a separate test if they
should remain covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4eb1d72a-594c-46bc-9a5b-6ffbeb62107d
📒 Files selected for processing (138)
src/features/api/folded-booking.tssrc/features/api/payment-processing/cancel.tssrc/features/api/payment-processing/classify.tssrc/features/api/payment-processing/create.tssrc/features/api/payment-processing/index.tssrc/features/api/payment-processing/items.tssrc/features/api/payment-processing/pricing.tssrc/features/api/payment-processing/recovery-decision.tssrc/features/api/payment-processing/recovery.tssrc/features/api/payment-processing/refunds.tssrc/features/api/payment-processing/replay.tssrc/features/api/payment-processing/store-refund.tssrc/features/api/sms-webhook.tssrc/features/api/webhooks.tssrc/features/public/ticket-payment.tssrc/shared/accounting/rows.tssrc/shared/booking-lines.tssrc/shared/booking.tssrc/shared/db/address-cache.tssrc/shared/db/attendees/activate.tssrc/shared/db/attendees/api.tssrc/shared/db/attendees/capacity/checks.tssrc/shared/db/attendees/create-batch.tssrc/shared/db/attendees/create.tssrc/shared/db/attendees/delete.tssrc/shared/db/attendees/order-parents.tssrc/shared/db/attendees/ordinary.tssrc/shared/db/attendees/queries.tssrc/shared/db/attendees/select.tssrc/shared/db/attendees/tokens.tssrc/shared/db/checkout-stages.tssrc/shared/db/listing-overview-stats.tssrc/shared/db/migrations/2026-07-17_checkout_stage_provider_id.tssrc/shared/db/migrations/registry.tssrc/shared/db/migrations/schema/tables-attendees.tssrc/shared/db/modifier-usage.tssrc/shared/db/processed-payments.tssrc/shared/db/prune.tssrc/shared/db/questions/attendee-answers/reads.tssrc/shared/db/system-notes.tssrc/shared/now.tssrc/shared/payment-helpers.tssrc/shared/payments.tssrc/shared/refund-ledger.tssrc/shared/square-provider.tssrc/shared/square.tssrc/shared/staged-checkout.tssrc/shared/stripe-provider.tssrc/shared/stripe.tssrc/shared/sumup-provider.tssrc/shared/sumup.tstest/e2e/accounting/drivers.tstest/features/api/payment-processing/create.test.tstest/features/api/payment-processing/recovery-decision.test.tstest/features/api/payment-processing/recovery.test.tstest/features/api/payment-processing/staged-refund-errors.test.tstest/features/api/payment-processing/staged-refunds.test.tstest/features/api/payment-processing/staged-runtime.helpers.tstest/features/api/payment-processing/staged-runtime.test.tstest/lib/db/attendees/create-attendee-atomic.test.tstest/lib/db/attendees/delete-attendee.test.tstest/lib/db/attendees/get-newest-attendees-raw.test.tstest/lib/db/attendees/select.test.tstest/lib/db/checkout-stage-schema.test.tstest/lib/db/migration-restore/verify.test.tstest/lib/db/migration-schema-guard.test.tstest/lib/payment-success-helpers.tstest/lib/server-api-packages.test.tstest/lib/server-attendees-list.test.tstest/lib/server-balance-payment-replay.test.tstest/lib/server-listings/show-actions-and-activity.test.tstest/lib/server-package-children.test.tstest/lib/server-payments-success-basic.test.tstest/lib/server-payments-success-refunds.test.tstest/lib/server-payments-success-replay.test.tstest/lib/server-payments/cancel-race.test.tstest/lib/server-payments/cancel.test.tstest/lib/server-payments/replay.test.tstest/lib/server-payments/success.test.tstest/lib/server-public/ticket-additional-coverage.test.tstest/lib/server-qr-book.test.tstest/lib/server-reservation/deposit-basics.test.tstest/lib/server-reservation/edge-cases.test.tstest/lib/server-reservation/helpers.tstest/lib/server-reservation/promo-addons.test.tstest/lib/server-webhook-dual-path.test.tstest/lib/server-webhooks/already-processed-rollback.test.tstest/lib/server-webhooks/can-pay-more-multi-ticket.test.tstest/lib/server-webhooks/can-pay-more-single-ticket.test.tstest/lib/server-webhooks/custom-questions-multi.test.tstest/lib/server-webhooks/custom-questions-single.test.tstest/lib/server-webhooks/customisable-days-pricing.test.tstest/lib/server-webhooks/modifier-refunds.test.tstest/lib/server-webhooks/multi-ticket-booking.test.tstest/lib/server-webhooks/multi-ticket-refunds.test.tstest/lib/server-webhooks/price-paid-calculation.test.tstest/lib/server-webhooks/refund-helper-functions.test.tstest/lib/server-webhooks/refund-logging.test.tstest/lib/server-webhooks/refund-skip-conditions.test.tstest/lib/server-webhooks/session-resolution.test.tstest/lib/server-webhooks/single-ticket-refunds.test.tstest/lib/server-webhooks/sumup.test.tstest/lib/square/fixtures.tstest/lib/square/provider.test.tstest/lib/square/retrieve-refund.test.tstest/lib/stripe/core.test.tstest/lib/stripe/provider.test.tstest/lib/stripe/webhook-setup.test.tstest/lib/webhook-price-signature/helpers.tstest/lib/webhook-price-signature/post-commit-recovery.test.tstest/lib/webhook-price-signature/stored-refund-and-ignore.test.tstest/shared/db/attendees/activate-staged-refusals.test.tstest/shared/db/attendees/activate-staged.helpers.tstest/shared/db/attendees/activate-staged.test.tstest/shared/db/orphan-attendees.test.tstest/shared/db/prune/checkout-stages.test.tstest/shared/db/system-notes.test.tstest/shared/now.test.tstest/shared/payment-helpers/dispatch.test.tstest/shared/payments.test.tstest/shared/refund-ledger-placeholder.test.tstest/shared/square-checkout-close.test.tstest/shared/square-provider.test.tstest/shared/square/payment-link-validation.test.tstest/shared/square/payment-link.test.tstest/shared/square/rest-transport.test.tstest/shared/staged-checkout.test.tstest/shared/stripe-checkout-close.test.tstest/shared/sumup-checkout-close.test.tstest/shared/sumup-provider.test.tstest/test-utils/checkout-stages.tstest/test-utils/checkout.tstest/test-utils/order-journey.tstest/test-utils/settings.tstest/test-utils/staged-payments.test.tstest/test-utils/staged-payments.tstest/test-utils/system-notes.tstest/test-utils/webhooks.ts
💤 Files with no reviewable changes (3)
- src/features/api/payment-processing/recovery-decision.ts
- test/shared/refund-ledger-placeholder.test.ts
- test/features/api/payment-processing/recovery-decision.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2750b67224
ℹ️ 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".
|
Outside-diff findings from the review are addressed in
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/square.ts (1)
629-646: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not recover from a wrong-resource delete response.
The
deleted.id/cancelledOrderIdmismatch is thrown inside thetry, so thecatchtreats it like a delete failure. If the follow-up order lookup is alreadyCOMPLETEDorCANCELED, this returns"paid"/"closed"even though Square reported the wrong payment link or order. Move the response validation outside the recovery block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/square.ts` around lines 629 - 646, The payment-link response validation must not be handled by the delete-failure recovery in the surrounding try/catch. In the payment-link deletion flow, retain the delete call inside the try, but move the deleted.id and deleted.cancelledOrderId checks after the catch so mismatched resources always throw instead of returning paid or closed based on fetchOrder.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/api/payment-processing/create.ts`:
- Around line 254-266: Replace the native validatedItems.map call in the
payment-processing creation flow with the repository’s curried map helper,
importing it according to the documented `#fp` conventions. Also update
test/shared/sumup.test.ts lines 101-103 to use the curried `#fp` filter helper
instead of native filter; both sites require direct changes.
In `@test/lib/stripe/core.test.ts`:
- Around line 223-226: Update the expiry assertions around sessions.create to
capture one Unix timestamp before invoking checkout, then reuse it for both
bounds. Use an inclusive lower-bound comparison against the captured timestamp
plus 30 minutes, while retaining the existing upper-bound window.
In `@test/test-utils/staged-payments.ts`:
- Around line 73-77: Wrap the activation-dependent staging flow in a try/finally
that always restores listings activated by the setup, including when
requiredMapValue throws for a missing listing. Update the staged-payment test
coverage with a mixed inactive-and-missing-listing regression case, and verify
cleanup leaves no temporary active state for subsequent tests.
In `@test/test-utils/webhooks.ts`:
- Around line 116-123: Update the staged-refund webhook assertion docblock near
verifyWebhookSignature and stripeApi.refundPayment so its default error
description refers to the couldn't-complete-your-booking response instead of the
saved-your-details message; retain the override guidance for scenario-specific
messages.
---
Outside diff comments:
In `@src/shared/square.ts`:
- Around line 629-646: The payment-link response validation must not be handled
by the delete-failure recovery in the surrounding try/catch. In the payment-link
deletion flow, retain the delete call inside the try, but move the deleted.id
and deleted.cancelledOrderId checks after the catch so mismatched resources
always throw instead of returning paid or closed based on fetchOrder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c09c3752-94ab-451f-a169-8dd9eca289e9
📒 Files selected for processing (60)
scripts/mutation/equivalent-mutants.txtsrc/features/api/payment-processing/create.tssrc/features/api/payment-processing/index.tssrc/features/api/payment-processing/recovery.tssrc/features/api/payment-processing/replay.tssrc/features/api/payment-processing/store-refund.tssrc/features/api/webhooks.tssrc/shared/booking-lines.tssrc/shared/db/checkout-stages.tssrc/shared/payment-helpers.tssrc/shared/square.tssrc/shared/staged-checkout.tssrc/shared/stripe-provider.tssrc/shared/stripe.tssrc/shared/sumup-provider.tssrc/shared/sumup.tstest/features/api/payment-processing/create.test.tstest/features/api/payment-processing/recovery.test.tstest/features/api/payment-processing/replay.test.tstest/features/api/payment-processing/staged-refund-errors.test.tstest/features/api/payment-processing/staged-refunds.test.tstest/features/api/payment-processing/staged-runtime.test.tstest/lib/db/attendees/select.test.tstest/lib/server-payments/replay.test.tstest/lib/server-payments/success.test.tstest/lib/server-reservation/deposit-basics.test.tstest/lib/server-reservation/edge-cases.test.tstest/lib/server-reservation/promo-addons.test.tstest/lib/server-webhooks/already-processed-rollback.test.tstest/lib/server-webhooks/can-pay-more-multi-ticket.test.tstest/lib/server-webhooks/can-pay-more-single-ticket.test.tstest/lib/server-webhooks/custom-questions-multi.test.tstest/lib/server-webhooks/custom-questions-single.test.tstest/lib/server-webhooks/customisable-days-pricing.test.tstest/lib/server-webhooks/extract-intent-redirect.test.tstest/lib/server-webhooks/modifier-refunds.test.tstest/lib/server-webhooks/multi-ticket-booking.test.tstest/lib/server-webhooks/multi-ticket-refunds.test.tstest/lib/server-webhooks/refund-skip-conditions.test.tstest/lib/server-webhooks/session-resolution.test.tstest/lib/server-webhooks/single-ticket-refunds.test.tstest/lib/stripe/core.test.tstest/lib/webhook-price-signature/helpers.tstest/lib/webhook-price-signature/post-commit-recovery.test.tstest/lib/webhook-price-signature/stored-refund-and-ignore.test.tstest/lib/webhook-price-signature/trusted-and-mismatch.test.tstest/shared/booking-lines.test.tstest/shared/db/attendees/activate-staged-refusals.test.tstest/shared/db/attendees/activate-staged.helpers.tstest/shared/db/attendees/activate-staged.test.tstest/shared/db/prune/checkout-stages.test.tstest/shared/payment-helpers/dispatch.test.tstest/shared/square-checkout-close.test.tstest/shared/staged-checkout.test.tstest/shared/sumup-checkout-close.test.tstest/shared/sumup.test.tstest/test-utils/settings.tstest/test-utils/staged-payments.test.tstest/test-utils/staged-payments.tstest/test-utils/webhooks.ts
💤 Files with no reviewable changes (1)
- test/lib/server-webhooks/extract-intent-redirect.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 118facfa03
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/square-provider.ts (1)
115-119: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject malformed Square payment webhooks instead of returning
null.For a Square payment event,
order_idandidare required provider fields. Returningnullclassifies malformed data as an unrelated session and may acknowledge a paid webhook without retrying it. Throw explicitly or validate the payload at this boundary.Proposed fix
- if (!orderId || !paymentId) return Promise.resolve(null); + if (!orderId || !paymentId) { + throw new Error("Square payment webhook is missing order_id or id"); + }As per coding guidelines, “Missing expected fields from external data must fail at the boundary through Valibot validation or an explicit throw.” <coding_guidelines>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/square-provider.ts` around lines 115 - 119, Update the Square payment webhook handling around the orderId and paymentId extraction to reject malformed payloads when either required provider field is missing or invalid. Replace the Promise.resolve(null) path with an explicit throw or the established Valibot boundary validation, while preserving null handling for unrelated, valid events.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/square-payments.ts`:
- Around line 48-58: Update the payment validation condition in the Square
payment helper to reject payments with any refund, including partial refunds, by
requiring refunded_money.amount to equal zero and validating its currency
against the order currency. Preserve the existing checks for payment identity,
completion status, order association, amount type/range, and currency.
In `@src/shared/staged-checkout.ts`:
- Around line 139-146: Update paidCheckoutBookingsOrNull to replace the native
intent.items.map call with the repository’s curried `#fp` map helper, adding the
helper import according to the established FP import conventions while
preserving the existing listing ID deduplication behavior.
---
Outside diff comments:
In `@src/shared/square-provider.ts`:
- Around line 115-119: Update the Square payment webhook handling around the
orderId and paymentId extraction to reject malformed payloads when either
required provider field is missing or invalid. Replace the Promise.resolve(null)
path with an explicit throw or the established Valibot boundary validation,
while preserving null handling for unrelated, valid events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bede89fa-a112-4ed3-b231-2253b7b6c89d
📒 Files selected for processing (39)
src/features/admin/attendee-refunds.tssrc/features/admin/refunds/provider.tssrc/features/admin/refunds/waves.tssrc/features/api/payment-processing/create.tssrc/features/api/payment-processing/index.tssrc/features/api/payment-processing/refunds.tssrc/features/api/payment-processing/replay.tssrc/features/api/payment-processing/store-refund.tssrc/features/api/webhook-types.tssrc/features/api/webhooks.tssrc/fp.tssrc/shared/payments.tssrc/shared/square-payments.tssrc/shared/square-provider.tssrc/shared/square.tssrc/shared/staged-checkout.tssrc/shared/stripe-provider.tssrc/shared/sumup-provider.tstest/e2e/accounting/drivers.tstest/features/admin/refunds/provider-batch.test.tstest/features/admin/refunds/provider.test.tstest/features/api/payment-processing/recovery.test.tstest/features/api/payment-processing/replay.test.tstest/features/api/payment-processing/staged-refund-errors.test.tstest/features/api/payment-processing/staged-refunds.test.tstest/lib/server-payments/replay.test.tstest/lib/server-webhooks/refund-helper-functions.test.tstest/lib/square/provider.test.tstest/lib/square/retrieve-refund.test.tstest/lib/stripe/core.test.tstest/lib/stripe/provider.test.tstest/shared/square-checkout-close.test.tstest/shared/staged-checkout.test.tstest/shared/sumup-provider.test.tstest/shared/sumup.test.tstest/test-utils/refund-routes.tstest/test-utils/staged-payments.test.tstest/test-utils/staged-payments.tstest/test-utils/webhooks.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 244987947b
ℹ️ 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".
|
The latest outside-diff Square finding is resolved in |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/admin/attendee-refunds.ts (1)
202-211: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove bulk refund messages into the locale catalog.
The updated
msgtext is user-facing copy insrc/features/admin/attendee-refunds.ts. Add parameterized entries tosrc/locales/en/attendees.jsonand access them throught(...)instead of constructing the copy here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/admin/attendee-refunds.ts` around lines 202 - 211, Move the user-facing refund summary text from the msg construction in the bulk refund flow into parameterized entries in the attendees locale catalog. Add separate translations for the remaining and completed cases, including refundedCount, pendingCount, problemCount, errorNote, and remaining parameters as needed, then use t(...) to select the appropriate entry while preserving the existing message content and branching behavior.Source: Coding guidelines
test/shared/square-provider.test.ts (1)
35-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the Square order state configurable.
sessionMocksalways returnsstate: "COMPLETED"at Line 56, but the refactored test at Lines 178-183 is for anOPENorder with aCOMPLETEDpayment. That change removes the setup for the branch being tested. Add anorderStateoption and pass"OPEN"for this case, or retain the inline order stub.Also applies to: 178-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shared/square-provider.test.ts` around lines 35 - 80, The sessionMocks helper hardcodes the Square order state, preventing tests from modeling non-completed orders. Add an orderState option to sessionMocks with the existing completed state as its default, use it for the returned order state, and pass "OPEN" in the test covering an open order with a completed payment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test-utils/refund-routes.ts`:
- Around line 141-145: Update the exported withRefundMock helper signature to
explicitly declare a Promise<void> return type, while preserving its existing
parameters and implementation behavior.
---
Outside diff comments:
In `@src/features/admin/attendee-refunds.ts`:
- Around line 202-211: Move the user-facing refund summary text from the msg
construction in the bulk refund flow into parameterized entries in the attendees
locale catalog. Add separate translations for the remaining and completed cases,
including refundedCount, pendingCount, problemCount, errorNote, and remaining
parameters as needed, then use t(...) to select the appropriate entry while
preserving the existing message content and branching behavior.
In `@test/shared/square-provider.test.ts`:
- Around line 35-80: The sessionMocks helper hardcodes the Square order state,
preventing tests from modeling non-completed orders. Add an orderState option to
sessionMocks with the existing completed state as its default, use it for the
returned order state, and pass "OPEN" in the test covering an open order with a
completed payment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14805a51-fff4-477d-b76d-63d73f1a42c5
📒 Files selected for processing (31)
src/features/admin/attendee-refunds.tssrc/features/api/payment-processing/index.tssrc/features/api/payment-processing/pricing.tssrc/features/api/payment-processing/refunds.tssrc/features/api/payment-processing/store-refund.tssrc/locales/en/attendees.jsonsrc/shared/db/checkout-stages.tssrc/shared/db/migrations/2026-07-18_checkout_stage_refund_spec.tssrc/shared/db/migrations/registry.tssrc/shared/db/migrations/schema/tables-attendees.tssrc/shared/payment-idempotency.tssrc/shared/refund-reasons.tssrc/shared/square-payments.tssrc/shared/square.tssrc/shared/staged-checkout.tssrc/shared/stripe.tstest/features/api/payment-processing/create.test.tstest/features/api/payment-processing/recovery.test.tstest/features/api/payment-processing/staged-refund-errors.test.tstest/features/api/payment-processing/staged-refunds.test.tstest/lib/db/checkout-stage-schema.test.tstest/lib/db/migration-schema-guard.test.tstest/lib/server-refunds.test.tstest/lib/stripe-mock/ports.test.tstest/lib/stripe/core.test.tstest/shared/db/attendees/activate-staged-refusals.test.tstest/shared/db/attendees/activate-staged.test.tstest/shared/db/migration-round-trip-budget.test.tstest/shared/square-provider.test.tstest/test-utils/checkout-stages.tstest/test-utils/refund-routes.ts
💤 Files with no reviewable changes (1)
- test/lib/stripe-mock/ports.test.ts
|
The outside-diff malformed Square webhook finding is resolved in |
|
The outside-diff bulk-refund copy finding is resolved in |
|
The outside-diff Square fixture finding is resolved in |
There was a problem hiding this comment.
💡 Codex Review
tickets/src/features/api/payment-processing/committed-entries.ts
Lines 50 to 55 in e5c05a5
Preserve paid-now amounts for deposit checkouts
When a public reservation/deposit checkout completes, bookingFactsFromOrder records sale legs for the full ticket value and a smaller payment leg for the deposit actually collected, but this reconstruction reads attendee.price_paid from the sale ledger projection. The confirmation email, admin “Amount paid”, and webhook price_paid field therefore report the full ticket price even though remaining_balance is still positive, overstating what the customer paid for any deposit booking. Keep using the per-line charged amounts for price_paid when rebuilding the just-committed entries.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dafc381883
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24dcf46829
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f94e69cb44
ℹ️ 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".
…-runtime # Conflicts: # scripts/mutation/equivalent-mutants.txt # src/features/api/payment-processing/index.ts # src/features/api/payment-processing/items.ts # test/features/api/payment-processing/index/refunds.test.ts # test/features/api/payment-processing/items.test.ts
stefan-burke
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e267accfb
ℹ️ 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".
| stored.start_at === expected.startAt && | ||
| stored.end_at === expected.endAt && |
There was a problem hiding this comment.
When a daily listing's duration_days is changed after the checkout stage is written but before the buyer pays, the staged zero-quantity row still has the old end_at, while activation rebuilds expected from the current listing facts. This exact start_at/end_at match makes stagedLinesMatch fail and refunds the paid session as a stage_mismatch/unexpected_error, even though the later capacity check could validate the new range and the booking could be honoured. Match the immutable booking slot separately from the mutable date range, or update the range during activation.
Useful? React with 👍 / 👎.
…-runtime # Conflicts: # scripts/mutation/equivalent-mutants.txt # src/shared/square-provider.ts # src/shared/square.ts # test/integration/server/webhooks/square.test.ts # test/shared/square-provider.test.ts # test/shared/string-entries.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6730daad5b
ℹ️ 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 session = await provider.retrieveSession(sessionId); | ||
| const session = await provider.retrieveSession(sessionId, "callback"); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier refund-provider fix is that this callback path still retrieves the session through the currently active provider before it ever looks at the stored checkout stage. When an operator changes the active provider while a hosted checkout is still open, a paid session created by the old provider is reported as “not found” here, and the buyer’s redirect cannot create the ticket even though checkout_stages records the provider that owns the session; old-provider webhooks are similarly unable to complete it until scheduled recovery eventually runs. Load the stage for staged session ids and retrieve with stage.provider instead of the current setting.
Useful? React with 👍 / 👎.
| if (ids.length > 1) { | ||
| throw new Error(`Square order ${order.id} has multiple tenders`); |
There was a problem hiding this comment.
When a Square order has more than one tender, this throw is reached by both pending cleanup (closePaymentLink) and paid-stage recovery (retrieveSession(..., "recovery")). recoverCheckoutStage catches the error and only defers the same row, so a checkout whose redirect/webhook were missed but whose Square order has a failed tender plus a completed tender will never be marked paid or processed by scheduled recovery. Use a bounded completed-tender lookup or a stored tender id instead of making multiple tenders a permanent retry.
Useful? React with 👍 / 👎.
| logDebug("Payment", "Refund issued"); | ||
| return result; | ||
| } | ||
| if (result === "pending") return result; |
There was a problem hiding this comment.
Fresh evidence after the idempotency-key fix is that a staged Stripe refund that first returns pending exits here without storing a refund id or switching future recovery to an inspect path. The next staged retry calls refunds.create again with the same stable key, and Stripe documents that same-key retries return the saved first response body (https://docs.stripe.com/api/idempotent_requests), so a refund that later succeeds can keep looking pending locally until the key ages out; after that, the replay may be treated as a new create/failure while the original refund was already accepted. Persist/inspect the pending refund instead of replaying the create request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a742ff4dd
ℹ️ 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".
| }: CreatePaidCheckoutInput): Promise<PaidCheckoutResult> => { | ||
| const bookings = await paidCheckoutBookingsOrNull(intent); | ||
| if (bookings === null) return { type: "sold_out" }; | ||
| const available = await attendeesApi.checkBatchAvailability( |
There was a problem hiding this comment.
Fresh evidence in this version is that paidCheckoutBookingsOrNull() can return real listing facts, but this second availability read still re-queries listings and checkBatchAvailabilityImpl throws Listing not found if the listing is deleted in that small window before provider checkout creation. That exception is not caught here, so the public submit path returns a 500 even though no checkout exists and the normal outcome should be the same handled sold-out response.
Useful? React with 👍 / 👎.
| const inspectAfterFirst = provider.refundRetryMode === "inspect-after-first"; | ||
| if ( | ||
| inspectAfterFirst && | ||
| !(await claimPaymentRefundAttempt(provider.type, paymentReference)) |
There was a problem hiding this comment.
For inspect-after-first providers, if the worker dies after this durable claim succeeds but before provider.refundPayment() is actually called, every later retry takes the already-claimed branch and only inspects provider state. With SumUp that leaves a still-SUCCESSFUL transaction reported as pending forever, so staged/admin refunds can remain stuck without ever submitting the refund or surfacing a terminal failure; the claim needs proof that the non-idempotent POST started, or stale pre-submit claims need an explicit recovery/alert path.
Useful? React with 👍 / 👎.
Codex review pointed out that returning false for a refund response missing its object/id/status silently swallowed a Square contract break as an ordinary not-yet-refunded payment — the codebase's offensive-programming rule says a missing documented field is a hard no that must fail at the boundary. refundPayment now throws when the 200 response lacks a refund object, id, or status; withClient contains the throw into a logged false, so the bad response is diagnosable instead of silently false. PENDING/FAILED/unknown statuses still return false silently (only a present-but-not-confirmed status is a normal not-yet-refunded outcome). Adds a test asserting the boundary error log; the existing fails-safely tests still hold (the throw is contained to the same false outcome). The pending-result union Codex also suggested (propagate a pending refund id instead of false) is the staged-checkout/callback work this PR was told not to introduce and is recorded in TODO.md against #1853/#1905. Refreshed the surviving equivalent-mutant line numbers in equivalent-mutants.txt (the helper removal shifted result ?? false and locations ?? []).
Codex follow-up (thread 3): the boundary throw lived inside withClient, which catches non-PaymentUserError exceptions and returns null, so it was architecturally the same as a logged false — an indirect throw-and-catch rather than a real loud failure. Replaced it with an explicit logError + return false, matching the missing-amount case just above it: the malformed response is still diagnosed at the boundary under E_SQUARE_REFUND and still fails safely to false (never reports a refund that was never confirmed), now without the swallowed-throw indirection. Propagating the throw outside withClient (a loud/errored failure) would change the contract from fail-safely-contained to fail-loudly, which is the per-path staged-checkout/callback resolution this PR was told not to introduce and is recorded in TODO.md against #1853 and #1905. Exhaustive mutation on square.ts is now 100% (286 killed, 3 genuine equivalents suppressed).
Codex review pointed out that returning false for a refund response missing its object/id/status silently swallowed a Square contract break as an ordinary not-yet-refunded payment — the codebase's offensive-programming rule says a missing documented field is a hard no that must fail at the boundary. refundPayment now throws when the 200 response lacks a refund object, id, or status; withClient contains the throw into a logged false, so the bad response is diagnosable instead of silently false. PENDING/FAILED/unknown statuses still return false silently (only a present-but-not-confirmed status is a normal not-yet-refunded outcome). Adds a test asserting the boundary error log; the existing fails-safely tests still hold (the throw is contained to the same false outcome). The pending-result union Codex also suggested (propagate a pending refund id instead of false) is the staged-checkout/callback work this PR was told not to introduce and is recorded in TODO.md against #1853/#1905. Refreshed the surviving equivalent-mutant line numbers in equivalent-mutants.txt (the helper removal shifted result ?? false and locations ?? []).
Codex follow-up (thread 3): the boundary throw lived inside withClient, which catches non-PaymentUserError exceptions and returns null, so it was architecturally the same as a logged false — an indirect throw-and-catch rather than a real loud failure. Replaced it with an explicit logError + return false, matching the missing-amount case just above it: the malformed response is still diagnosed at the boundary under E_SQUARE_REFUND and still fails safely to false (never reports a refund that was never confirmed), now without the swallowed-throw indirection. Propagating the throw outside withClient (a loud/errored failure) would change the contract from fail-safely-contained to fail-loudly, which is the per-path staged-checkout/callback resolution this PR was told not to introduce and is recorded in TODO.md against #1853 and #1905. Exhaustive mutation on square.ts is now 100% (286 killed, 3 genuine equivalents suppressed).
Codex review pointed out that returning false for a refund response missing its object/id/status silently swallowed a Square contract break as an ordinary not-yet-refunded payment — the codebase's offensive-programming rule says a missing documented field is a hard no that must fail at the boundary. refundPayment now throws when the 200 response lacks a refund object, id, or status; withClient contains the throw into a logged false, so the bad response is diagnosable instead of silently false. PENDING/FAILED/unknown statuses still return false silently (only a present-but-not-confirmed status is a normal not-yet-refunded outcome). Adds a test asserting the boundary error log; the existing fails-safely tests still hold (the throw is contained to the same false outcome). The pending-result union Codex also suggested (propagate a pending refund id instead of false) is the staged-checkout/callback work this PR was told not to introduce and is recorded in TODO.md against #1853/#1905. Refreshed the surviving equivalent-mutant line numbers in equivalent-mutants.txt (the helper removal shifted result ?? false and locations ?? []).
Codex follow-up (thread 3): the boundary throw lived inside withClient, which catches non-PaymentUserError exceptions and returns null, so it was architecturally the same as a logged false — an indirect throw-and-catch rather than a real loud failure. Replaced it with an explicit logError + return false, matching the missing-amount case just above it: the malformed response is still diagnosed at the boundary under E_SQUARE_REFUND and still fails safely to false (never reports a refund that was never confirmed), now without the swallowed-throw indirection. Propagating the throw outside withClient (a loud/errored failure) would change the contract from fail-safely-contained to fail-loudly, which is the per-path staged-checkout/callback resolution this PR was told not to introduce and is recorded in TODO.md against #1853 and #1905. Exhaustive mutation on square.ts is now 100% (286 killed, 3 genuine equivalents suppressed).
1 participant
What changed
Paid checkouts now save an unfinished attendee and booking before opening the payment page. The booking has zero quantity, so it never holds capacity.
After the provider confirms payment, the same records activate in one database transaction. Capacity, add-on stock, payment history, ticket access, personal details, and ledger entries either all finish together or none do.
If the booking cannot finish, the checkout enters a refund-only state before the provider is contacted. Refund attempts are stored so retries cannot issue a duplicate provider refund. Stripe, Square, and SumUp share the same pending, failed, and completed refund flow.
Scheduled maintenance now recovers paid and refunding checkouts when a callback or browser redirect was missed. Recovery uses durable retry times, provider-specific request budgets, and follow-up runs so one failed checkout cannot block the queue.
Payment callbacks replay stored final results even when the provider state later changes. Cancellation only offers a new checkout after the old session is confirmed closed. Cleanup keeps live checkout stages, hidden attendees, and SumUp metadata until payment or refund recovery has finished.
The design documents record the remaining release-version, key-rotation, backup, host-move, and rollback work. Those deployment safeguards are not part of this PR.
Operating limits
This supports the current running version only. Deployments, host moves, and restores do not preserve open payments. No rollback fence, stage revision, backup certification, admin payment lock, or old-version reconciliation is included.
Verification