Skip to content

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

Closed
stefan-burke wants to merge 31 commits into
mainfrom
claude/branch-review-comparison-pcwbvd
Closed

stefan-burke wants to merge 31 commits into
mainfrom
claude/branch-review-comparison-pcwbvd

Conversation

@stefan-burke

@stefan-burke stefan-burke commented Jul 13, 2026

Copy link
Copy Markdown
Member

This adds staged checkout and hardens it end to end — including everything from the follow-up review (items 1–10) and every automated review finding. It targets main directly and folds in the earlier fix/committed-booking-refunds work, so it is one standalone change.

What staged checkout does

When someone pays, we now write their order to the database before sending them to the card page — as an attendee with quantity-zero booking rows plus a checkout_stages record. When the payment lands, it claims those exact rows and gives them their real quantities in one atomic step. No seat is ever held while someone pays: the first payment to land wins, and holding seats would invite bots (this is now written policy in AGENTS.md).

This closes the window where a completed payment could arrive and find no record to attach to. Every outcome — a ticket, a refund, or an order the operator must look at — has one place to resolve and one record already there to carry the result.

An unbookable order never reaches the card page

Before creating the provider session, we check the order still fits and every listing is still on sale. If not, the customer is told plainly ("Sorry, this can no longer be booked…") instead of paying and being refunded. The real capacity decision still happens at activation, with the real quantities.

How failures end safely

  • Changed booking lines (all rows still quantity zero, nothing live) take the keep-and-refund path with a clear reason.
  • Rows already given a real quantity may be a live booking, so that becomes a no-refund operator conflict: a loud alert, a note saying exactly what to check, and the received money posted on its own — the books say "we hold this customer's money" while the operator decides.
  • Transient errors retry on the provider's next delivery. A refund or ledger write that fails leaves the stage pending, so the retry re-runs the whole path; a settled refund reads back as success, so nothing refunds twice.
  • Money is recorded before it is exposed. The payment reference is stamped onto the record only after the ledger holds the money, so the refund button can never appear for a charge the books haven't seen — and the crash-heal path writes the reference back if an interruption lost it.
  • A stage pointing at a deleted attendee, or a staged attendee with no rows, is an impossible state and fails loudly instead of being papered over.

Mid-payment records are locked, and say so

  • The record page hides the Edit, Logistics, and Actions tabs while a checkout is pending (their URLs return "not found"), with a banner: "Checkout pending: the customer may still be paying."
  • Every attendee table shows "Payment in progress" instead of "No quantity", and CSV exports gain a "Checkout pending" column when any exported row is mid-payment.
  • Edits, merges, and deletes fail closed with a plain message. A record holding unreturned conflict money can't be deleted, merged, or stripped of its last line until the money is refunded.
  • A listing can't be deleted while it has a pending checkout — and if a delete races past that check, the pending order's rows survive and show as a read-only "Deleted listing" line, so the operator always sees what the customer paid for.
  • A note's link to the owner-only ledger pages shows as plain text to other admins — no link that only fails.

Provider lifecycle

  • The cancel page discards the staged details and closes the provider session, so an old tab can't pay for a checkout we no longer hold.
  • Stripe checkouts get a hard deadline (CHECKOUT_SESSION_EXPIRY_MINUTES, default 60); expired checkouts are discarded the moment Stripe reports them. SumUp checkouts that lapse take the same discard-and-try-again path as a declined card.
  • Setting up Stripe now removes any stray webhook endpoints already pointing at this site (left by a lost setup or a database restore) — a stray would fail signature checks on every delivery forever.

One accepted edge, by the owner's call

A paid checkout whose processing keeps failing for days could eventually have its staged record cleaned up. The money is never lost (it stays with the provider), and the operator is alerted on every failing delivery for days first — so this is documented in TODO.md rather than coded around.

Shared mechanisms

One attendee-purge mechanism now serves the single delete, the orphan purge, and the pending-checkout discard; one primary-pinned batch guard serves every mutation check; plus queryAllPrimary/queryOnePrimary, matchingIdSet, and one shared attendee-echo projection.

Verification

  • deno task precommit passes in full (typecheck including tests, strict lint, complete suite, 0% duplication, 100% line and branch coverage).
  • Every fix ships with a regression test that failed before it.
  • Every automated review thread is answered on this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR hardens staged-checkout activation and recovery, blocks administrative mutations during pending payment, improves staged-row cleanup and lookup behavior, adds structured conflict/refund outcomes, standardizes attendee projections, and aligns Stripe checkout expiry with webhook and cancellation cleanup.

Changes

Staged-checkout hardening

Layer / File(s) Summary
Hardening plan and outcome contracts
plan.md, TODO.md, src/features/api/payment-processing/*, src/shared/db/attendees/activate.ts
Defines staged-checkout fixes and adds structured activation failure, refund, and recovery outcomes.
Activation and attendee data integrity
src/shared/db/attendees/*, src/features/api/payment-processing/committed-entries.ts, test/shared/db/attendees/activate.test.ts, test/lib/processed-payments/locking.test.ts
Validates staged rows without throwing, preserves parent/child booking metadata, uses primary-pinned reads, and standardizes attendee defaults.
Stage storage and administrative guards
src/shared/db/checkout-stages.ts, src/features/admin/*, src/ui/templates/admin/attendee-page.tsx, src/locales/en/*, test/lib/server-attendees/*, test/shared/db/checkout-stages.test.ts
Adds nullable stage lookup, shared dependent-row cleanup, overbook-tolerant staging, pending-payment edit/merge guards, and related UI/test coverage.
Provider expiry lifecycle
src/shared/limits.ts, src/shared/payments.ts, src/shared/stripe*.ts, src/features/api/webhooks.ts, test/lib/server-payments/cancel.test.ts, test/lib/stripe/core.test.ts
Validates checkout expiry limits, configures Stripe expiration, supports provider session expiry, and discards staged rows on cancellation or expiry events.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Stripe
  participant Webhooks
  participant CheckoutStages
  participant PaymentProcessing
  Stripe->>Webhooks: checkout.session.expired
  Webhooks->>CheckoutStages: discardPendingCheckoutSessions
  CheckoutStages-->>Webhooks: staged rows removed
  Stripe->>PaymentProcessing: checkout completion
  PaymentProcessing->>CheckoutStages: getCheckoutStageOrNull
  PaymentProcessing-->>Stripe: handled or staged payment result
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the PR’s main theme: staged checkout before payment plus broader flow hardening.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/branch-review-comparison-pcwbvd

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9aabe20812

ℹ️ 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".

Comment thread src/features/api/payment-processing/store-refund.ts Outdated
Comment thread src/features/api/payment-processing/store-refund.ts
Comment thread src/shared/db/checkout-stages.ts Outdated
Comment thread src/shared/db/checkout-stages.ts Outdated
Comment thread src/shared/stripe.ts
Comment thread src/features/api/payment-processing/store-refund.ts
Comment thread src/features/api/payment-processing/store-refund.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/shared/db/attendees/activate.ts (1)

128-155: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Make the stage-active check atomic with the claim
findStageProblem runs before withTransaction, so a concurrent writer can flip a staged row live in that gap and activationStatement will still overwrite it because the UPDATE only matches on identifiers. Move the read into the transaction or add AND quantity = 0 and treat 0 rows affected as stage_active.

🤖 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/db/attendees/activate.ts` around lines 128 - 155, Make the
stage-active validation in the activation flow atomic with the database claim:
move findStageProblem into the withTransaction callback, or constrain
activationStatement to quantity = 0 and handle an update affecting zero rows as
stage_active. Update the surrounding activate logic while preserving the
existing failure response and transaction behavior.
🤖 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/admin/attendees-merge.ts`:
- Around line 48-50: Move MERGE_PENDING_CHECKOUT_MESSAGE into the appropriate
English locale JSON catalog under a descriptive key, then replace both
references to the constant with t("key") calls using the existing translation
mechanism. Remove the hardcoded constant while preserving the same message
behavior.

In `@src/features/api/payment-processing/store-refund.ts`:
- Around line 282-284: Update the staged validation flow around
getCheckoutStageOrNull and markCheckoutStage so a transient refund failure does
not mark the checkout stage as terminal before the refund outcome is known.
Leave the stage pending, or use an existing refund-pending state that allows
processPaymentSession to retry when result.refunded is false, while preserving
terminal handling for confirmed outcomes.

In `@src/shared/db/checkout-stages.ts`:
- Around line 154-159: Add a deterministic test for the lookup path handling
orphaned staged attendees, using the relevant checkout-stage lookup symbol to
create a staged row whose attendee is then deleted without removing the stage.
Assert the lookup returns null and ensure the test exercises the attendee_exists
=== null branch, including its recovery behavior.
- Around line 179-188: The attendeeIdsWithPendingStage lookup must use the
primary database to avoid replica lag when gating mutations. Replace queryAll
with queryBatchPrimary or the existing primary-only helper while preserving the
current SQL, parameters, empty-input fast path, and Set<number> result.

In `@src/shared/stripe.ts`:
- Around line 460-461: Update the exported expireCheckoutSession wrapper to
declare an explicit return type matching stripeApi.expireCheckoutSession(id),
while preserving its current behavior and delegation.

In `@test/lib/server-attendees/attendee-edit.test.ts`:
- Line 5: The raw listing_attendees quantity query is duplicated across attendee
tests; extract it into a shared `#test-utils` helper named getAttendeeQuantities,
then replace the direct queryAll usage in attendee-edit.test.ts and
merge-post.test.ts with that helper while preserving the existing result shape
and attendee ID parameter.

In `@test/lib/server-attendees/merge-post.test.ts`:
- Around line 154-166: Add a database-state assertion to the “refuses a
mid-payment target” test, using the same queryAll-based verification as the
sibling mid-payment-source test. Confirm the staged attendee row identified by
stage.attendeeId remains unchanged after the merge request, while preserving the
existing redirect and flash assertions.

In `@test/test-utils/db-helpers/processed-payments.ts`:
- Around line 46-50: Add a regression test for the processed payment session
finalization helper that first resolves a session, then calls the finalization
operation again and asserts it throws when rowsAffected is not 1. Reuse the
existing test setup and symbols around finalizeTestPaymentSession, preserving
current successful-finalization coverage.

---

Outside diff comments:
In `@src/shared/db/attendees/activate.ts`:
- Around line 128-155: Make the stage-active validation in the activation flow
atomic with the database claim: move findStageProblem into the withTransaction
callback, or constrain activationStatement to quantity = 0 and handle an update
affecting zero rows as stage_active. Update the surrounding activate logic while
preserving the existing failure response and transaction behavior.
🪄 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: cf09b603-2955-4fcd-b0fd-7620a9603eed

📥 Commits

Reviewing files that changed from the base of the PR and between 4236e09 and 9aabe20.

📒 Files selected for processing (39)
  • TODO.md
  • plan.md
  • src/features/admin/attendee-form-routes.ts
  • src/features/admin/attendee-page.ts
  • src/features/admin/attendees-merge.ts
  • src/features/api/payment-processing/committed-entries.ts
  • src/features/api/payment-processing/create.ts
  • src/features/api/payment-processing/index.ts
  • src/features/api/payment-processing/recovery-decision.ts
  • src/features/api/payment-processing/refunds.ts
  • src/features/api/payment-processing/store-refund.ts
  • src/features/api/webhooks.ts
  • src/locales/en/admin.json
  • src/locales/en/attendees.json
  • src/shared/db/attendees/activate.ts
  • src/shared/db/attendees/atomic-update.ts
  • src/shared/db/attendees/create.ts
  • src/shared/db/attendees/delete.ts
  • src/shared/db/attendees/order-parents.ts
  • src/shared/db/attendees/pii.ts
  • src/shared/db/checkout-stages.ts
  • src/shared/limits.ts
  • src/shared/payments.ts
  • src/shared/stripe-provider.ts
  • src/shared/stripe.ts
  • src/ui/templates/admin/attendee-page.tsx
  • test/lib/db/attendees/delete-attendee.test.ts
  • test/lib/server-attendees/attendee-detail.test.ts
  • test/lib/server-attendees/attendee-edit.test.ts
  • test/lib/server-attendees/helpers.ts
  • test/lib/server-attendees/merge-panel.test.ts
  • test/lib/server-attendees/merge-post.test.ts
  • test/lib/server-payment-staging.test.ts
  • test/lib/server-payments/cancel.test.ts
  • test/lib/stripe/core.test.ts
  • test/shared/db/attendees/activate.test.ts
  • test/shared/db/checkout-stages.test.ts
  • test/shared/limits.test.ts
  • test/test-utils/db-helpers/processed-payments.ts

Comment thread src/features/admin/attendees-merge.ts Outdated
Comment thread src/features/api/payment-processing/store-refund.ts Outdated
Comment thread src/shared/db/checkout-stages.ts Outdated
Comment thread src/shared/db/checkout-stages.ts Outdated
Comment thread src/shared/stripe.ts Outdated
Comment thread test/lib/server-attendees/attendee-edit.test.ts Outdated
Comment thread test/lib/server-attendees/merge-post.test.ts
Comment thread test/test-utils/db-helpers/processed-payments.ts

Copy link
Copy Markdown
Member Author

Two notes on items without an inline thread:

  • CodeRabbit's outside-diff finding (activation stage-active check not atomic with the claim): fixed in 8f6aafb. The activation UPDATE now carries AND quantity = 0 in its own WHERE, so a row flipped live in the gap since the pre-check misses the write; a refused claim re-checks the rows before blaming stock, so a race classifies as a stage conflict (money held for the operator) rather than a refundable capacity failure.
  • The failing merge-check: it tries to merge this branch into main, and the conflicts it hits (trusted-and-mismatch.test.ts, listing-overview-stats.test.ts, …) are between the base branch (fix/committed-booking-refunds) and main — files this stacked PR doesn't touch. It will clear once the parent branch (Stage paid bookings until payment succeeds #1764) merges main; resolving them here would drag the parent's conflict resolution into this PR.

Generated by Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/db-helpers/attendees.ts`:
- Around line 108-118: Update getAttendeeQuantities to add an explicit
deterministic ORDER BY clause to the listing_attendees query, using a stable row
identifier so results have consistent ordering when multiple rows exist. Keep
the selected fields and attendeeId filtering unchanged.
🪄 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: 3271347e-74d5-4559-ab48-6360d89d91a1

📥 Commits

Reviewing files that changed from the base of the PR and between 9aabe20 and 8f6aafb.

📒 Files selected for processing (16)
  • TODO.md
  • src/features/admin/attendees-merge.ts
  • src/features/api/payment-processing/index.ts
  • src/features/api/payment-processing/store-refund.ts
  • src/locales/en/admin.json
  • src/shared/db/attendees/activate.ts
  • src/shared/db/checkout-stages.ts
  • src/shared/limits.ts
  • src/shared/stripe.ts
  • test/lib/processed-payments/locking.test.ts
  • test/lib/server-attendees/attendee-edit.test.ts
  • test/lib/server-attendees/merge-post.test.ts
  • test/lib/server-payment-staging.test.ts
  • test/shared/db/checkout-stages.test.ts
  • test/shared/limits.test.ts
  • test/test-utils/db-helpers/attendees.ts

Comment thread test/test-utils/db-helpers/attendees.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f6aafbfd2

ℹ️ 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".

Comment thread src/features/api/payment-processing/store-refund.ts Outdated
Comment thread src/features/api/payment-processing/store-refund.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/store-refund.ts`:
- Around line 225-234: In the refund flow, move markCheckoutStage(session.id,
"failed") to execute after tryRefund and recordPlaceholderRefund complete. Keep
stampStagedPaymentId before the refund attempt, and ensure the stage is marked
failed only after provider and placeholder-refund operations succeed, matching
the failStagedValidation/stagedConflict ordering.

In `@test/lib/server-payments/cancel.test.ts`:
- Around line 60-62: Update the cancel-payment tests around stubExpire and the
try blocks to declare the stub() result with using, relying on Symbol.dispose
for automatic restoration. Remove the manual restore/finally handling and
preserve the existing test behavior and assertions.
🪄 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: 2d38c279-7209-4c2e-b432-47e2e131aa19

📥 Commits

Reviewing files that changed from the base of the PR and between 8f6aafb and 5e00658.

📒 Files selected for processing (12)
  • src/features/api/payment-processing/committed-entries.ts
  • src/features/api/payment-processing/index.ts
  • src/features/api/payment-processing/store-refund.ts
  • src/shared/db/attendees/atomic-update.ts
  • src/shared/db/checkout-stages.ts
  • src/shared/db/client.ts
  • src/shared/db/listing-prices.ts
  • test/lib/server-payment-staging.test.ts
  • test/lib/server-payments/cancel.test.ts
  • test/test-utils/db-helpers/attendees.ts
  • test/test-utils/db-helpers/processed-payments.ts
  • test/ui/templates/admin/attendee-detail.test.ts

Comment thread src/features/api/payment-processing/store-refund.ts Outdated
Comment thread test/lib/server-payments/cancel.test.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e00658830

ℹ️ 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".

Comment thread src/features/api/payment-processing/store-refund.ts Outdated
Comment thread src/features/api/payment-processing/store-refund.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/lib/server-payment-staging.test.ts`:
- Around line 557-565: Replace the manual try/finally disposal in the staged
refund tests with using declarations for the stubSuccessfulRefund result,
matching the existing disposable stub pattern. Apply the same change to checkout
and refund values created by stubCheckout or stubSuccessfulRefund in the
affected tests only when those helpers return stub()-based disposables, and
remove the corresponding restore calls and try/finally wrappers.
🪄 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: 0e5b4efe-4b25-4c16-87ac-adc2d9c507df

📥 Commits

Reviewing files that changed from the base of the PR and between 5e00658 and ea9fec6.

📒 Files selected for processing (5)
  • src/features/api/payment-processing/index.ts
  • src/features/api/payment-processing/store-refund.ts
  • src/shared/db/checkout-stages.ts
  • test/lib/server-payment-staging.test.ts
  • test/lib/server-payments/cancel.test.ts

Comment thread test/lib/server-payment-staging.test.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9f4c09936b

ℹ️ 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".

Comment thread src/features/api/payment-processing/store-refund.ts Outdated
Comment thread src/features/api/payment-processing/index.ts Outdated
Comment thread src/features/admin/attendee-form-routes.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

validatedItems,
pricingIntent,
pricedOrder,
preparedTicketToken,
stage,
);

P2 Badge Recheck a deleted stage before refunding it

This passes a stage snapshot that may have been deleted by an operator while the payment was being processed. In that race, activateStagedBooking sees no staged lines and reports stage_mismatch; storeRefundedBooking then reloads no stage and creates a refunded placeholder, even though the documented delete path says removing a pending stage lets a late payment book fresh from the signed intent. The result is a valid paid checkout being refunded instead of booked solely because the delete landed after this read.

ℹ️ 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".

Comment on lines +323 to +327
await recordPlaceholderRefund(
placeholderFacts(session, stage.attendeeId, listingId),
"listing_closed",
true,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep staged validation ledger failures retryable

In this new staged-validation path, recordPlaceholderRefund catches ledger write failures and reports them as { posted: false }, but the result is ignored. When a listing closes or is deactivated mid-payment and the provider refund succeeds but the payment/refund ledger round-trip is not stored, this code still writes the note, marks the stage failed, and lets processPaymentSession record a terminal outcome; later deliveries then replay “already processed” and cannot repair the attendee ledger, leaving the operator’s record missing or overstating the refunded cash.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 9d9215c. The closed-listing path now verifies its ledger round-trip through the same shared recordStagedMoneyOrThrow the conflict path uses: a failed write throws before the note and the stage flip, so the stage stays pending and the redelivery re-runs the whole path (the settled refund reads back as refunded, so nothing double-refunds). The regression blocks the payment leg's reference, proves the throw and the pending stage, then repairs the collision and drives the redelivery to the recorded round-trip and a resolved stage.


Generated by Claude Code

Comment on lines +166 to +170
const healedAttendeeId = await resolvePendingStage(sessionId);
if (healedAttendeeId !== null) {
await createSystemNote(
healedAttendeeId,
healedStageNote(paymentReference),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Leave the crash-healing note before resolving

When the ledger preflight finds an orphaned event with a still-pending stage, this call flips the stage to failed before the explanatory note is inserted. If createSystemNote hits a write failure after the update, the next delivery sees no pending stage, returns “already handled,” and never writes the note, so the operator loses the only explanation for the retained/refunded/conflicted staged record.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 9d9215c. The heal reads the pending stage's attendee first (pendingStageAttendeeIdOrNull), writes the note, and only then resolves the stage — so a half-done heal repeats the note on the next delivery instead of losing it forever. A duplicate note beats a permanently missing explanation.


Generated by Claude Code

Comment on lines +390 to +394
const { posted } = await recordPlaceholderRefund(
placeholderFacts(session, stage.attendeeId, listingId),
"stage_active",
false,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make conflict refunds ledgerable

For a stage_active conflict this stores only a cash payment leg, while the stamped payment reference makes the normal admin refund action available on the now-live row. If the operator follows the note and refunds this charge in-app, recordAttendeeRefund refuses to post the ledger refund because the attendee account has only a credit and no balancing sale, so the provider refund succeeds but the app returns refund_not_recorded and leaves the ledger unreconciled.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 9d9215c. The admin refund guard now recognises pure held cash — covered legs that are all provider payments with no sale — as cleanly reversible, since the reversal returns exactly what we hold and leaves the account at zero. The regression seeds a conflict's held payment and proves the in-app refund posts its refund_cash reversal instead of stranding as refund_not_recorded. Every other shape (still owing, credit mixed with sales) keeps the old fully-paid guard and goes to a manual adjustment.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

On Codex's review-body finding ("Recheck a deleted stage before refunding it"): resolved in 9d9215c. When activation finds nothing staged any more — the operator deleted the record between the stage read and the claim, the one sanctioned mid-payment mutation — it now reports the stage gone rather than "changed", and createAttendeeForSession falls through to its own fresh create instead of refunding a bookable payment. The regression hands the production compose chain a genuinely stale snapshot for a deleted record and proves a brand-new attendee books and the payment finalizes against it, honouring the documented delete-mid-payment contract ("a late payment books fresh from the signed order").


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9d9215c86f

ℹ️ 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".

Comment on lines +126 to +128
if (!posted) {
throw new Error(
`Could not record session ${session.id}'s money in the ledger`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make staged refund ledger writes atomic

When refunded is true, recordPlaceholderRefund can commit the placeholder payment leg and then return { posted: false } if the following account read or refund_cash post fails. Throwing here leaves the stage pending, but after the stale reservation is reclaimed the ledger preflight sees that booking event as orphaned, resolves the stage, and returns “already handled” without ever writing the missing refund leg. A provider-refunded staged checkout can therefore remain recorded as held cash unless the payment and refund legs land atomically or replay can detect the incomplete placeholder refund.

Useful? React with 👍 / 👎.

// "already handled" (the conflicted rows never carry this session's event
// group, so the leg reads as orphaned money) — never a double post, and the
// preflight resolves the leftover pending stage (resolvePendingStage).
await stampStagedPaymentId(stage, session, intent);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Delay exposing refunds until conflict money is recorded

In the stage_active path, this stamps payment_id before recordStagedMoneyOrThrow has posted the held-payment leg. If that ledger write throws, the stage stays pending for retry, but the attendee now has a payment reference and a quantity-1 line, so the Actions tab's existing refund visibility checks can offer an in-app refund while the payment flow is still unresolved. If the operator refunds in that window, the later retry still records the session as held cash (refunded=false), leaving the ledger out of sync with the provider refund.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 21ed057. Both terminal staged-refund paths (recordHeldStagedMoney and storeRefundedBooking) now post the held-payment leg before stamping the provider payment reference. A failed post throws first, so the still-pending record carries no reference — the Actions tab has no charge to offer an in-app refund against while the money is unrecorded, closing the out-of-sync window you described. Regression test: the blocked-ledger conflict path asserts no payment reference is stamped after the throw (expectNoStampedPayment).


Generated by Claude Code

Comment on lines 249 to 253
await recordPlaceholderRefund(
{
amount: session.amountTotal,
attendeeId,
eventId: session.id,
listingId,
occurredAt: businessTime(session),
},
placeholderFacts(session, attendeeId, listingId),
spec.code,
refunded,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor failed placeholder ledger posts

In the keep-and-refund path for capacity, price, deleted-listing, or stage_mismatch failures, recordPlaceholderRefund reports ledger write failures as { posted: false }, but this result is ignored. When a staged checkout's provider refund succeeds but the payment/refund ledger round-trip is not stored, this still writes the note, marks the stage failed, and lets processPaymentSession record a terminal failure; later deliveries then replay the failure/resolved stage and cannot repair the retained quantity-0 attendee's missing ledger legs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 21ed057. storeRefundedBooking now captures recordPlaceholderRefund's {posted} and, on a staged order, throws when the post fails — leaving the stage pending so the provider's next delivery re-posts (the settled refund reads back as refunded, so it never refunds twice). The no-stage path can't retry without minting a duplicate placeholder beside the one it already created, so it still proceeds with the miss logged by postWithoutThrowing (its terminal-outcome design). Regression test: a kept-and-refund placeholder path throws and stays pending on a blocked ledger post, then records the round-trip and resolves on retry.


Generated by Claude Code

@stefan-burke
stefan-burke force-pushed the claude/branch-review-comparison-pcwbvd branch from 9d9215c to de47bff Compare July 13, 2026 15:18
@stefan-burke stefan-burke changed the title Harden the staged checkout flow (fixes from the branch review) Stage each paid order before payment, and harden the flow Jul 13, 2026
@stefan-burke
stefan-burke changed the base branch from fix/committed-booking-refunds to main July 13, 2026 15:19
Write a paid order as an attendee with quantity-zero booking rows and a
checkout_stages record BEFORE creating the provider session, then let the
payment claim those exact rows atomically when it lands. This closes the
window where a completed payment could find no record to attach to, and
gives every terminal outcome — ticket, refund, or operator conflict — one
place to resolve.

Deterministic activation problems now end safely instead of crash-looping
the webhook with the customer's money held and no record:

- Changed booking lines (all rows still quantity 0, nothing live) take the
  normal keep-and-refund path with a clear "the booking changed between
  checkout and payment" reason.
- Rows already given a real quantity may be a live booking, so that becomes
  a no-refund operator conflict: a loud alert, a note on the record saying
  what to check, and a recorded outcome so replays answer the same.
- Transient and system errors still retry on the provider's next delivery.

Held and refunded money is always on the record: every kept record for a
charged session stamps the provider's payment reference, and the ledger
shows the charge plus whatever refund actually happened. A failed refund
stays retryable (the stage resolves only after the money records land), and
a crash between the ledger post and the stage flip heals on the next
delivery.

Mid-payment records are protected from admin edits and merges (both fail
closed with a plain message; deleting still works). Checkouts can start on
overbooked or just-deactivated listings because quantity-zero staging skips
the capacity gate — real capacity is enforced at activation. Stripe
checkouts get a hard deadline (CHECKOUT_SESSION_EXPIRY_MINUTES, default 60);
expired sessions are discarded the moment Stripe reports them.

Shared mechanisms extracted along the way: a queryAllPrimary/queryOnePrimary
client helper, a matchingIdSet batch-membership helper, the claim-refusal
classification in its own directly-tested module, and one shared
attendee-echo projection.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu
@stefan-burke
stefan-burke force-pushed the claude/branch-review-comparison-pcwbvd branch from de47bff to ebca5d0 Compare July 13, 2026 15:39
A pending checkout stage losing its attendee mid-payment used to be
"recovered" by booking fresh from the signed order (createFresh / the
dangling-stage null return). That re-ran the whole create at the worst
moment — the customer has paid and left — trading a loud, catchable
failure for a second attempt with its own failure modes.

Make it an invariant instead: a pending stage and its rows only ever die
together. Enforce it, then throw if it is ever violated.

- Block deleting a mid-payment record (admin delete + delete-incomplete
  share one guard) and deleting a listing while it has a pending checkout
  (one listingDeleteError guard on all three delete paths). Abandoned
  checkouts still auto-clean via the prune / provider-expiry discard.
- Add checkout_stages to the orphan-purge cascade, so a listing deleted
  mid-checkout can't leave a dangling stage behind.
- findStageProblem (zero rows) and getCheckoutStageOrNull (dangling
  pending stage) now throw a contextful error instead of returning a
  "book fresh" signal; drop stage_gone from ActivationFailure and the
  createFresh recovery branch (createFresh stays for the genuine
  no-stage path).
- Fix the copy that told operators to "delete the record" as an escape
  hatch, now that deleting mid-payment is blocked.

Reworks the stage-gone tests to expect the loud throw and adds regression
tests for both delete guards, the listing pending-checkout check, and the
orphan stage cascade.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37492fb20e

ℹ️ 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".

Comment on lines +424 to +428
await recordHeldStagedMoney(
{ intent, listingId, session, stage },
"stage_active",
false,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Block no-quantity edits for held conflict cash

When a stage_active conflict reaches this call, it records only a provider payment leg (refunded=false) and deliberately no sale, so the now-editable live row still projects price_paid as 0. The Edit tab's no-quantity guards (isPaymentLockedLine/hasPaidLine) only block sale-backed paid lines, and the refund route later requires hasActiveBookingLine, so in this exact conflict an operator can mark the row no-quantity before refunding and then lose the normal in-app refund path while the provider cash remains held.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 99a13da. The save path now blocks a no-quantity edit while the attendee holds unreturned provider cash. New attendeeHoldsUnreturnedCash is a decryption-free ledger check that the attendee account has a positive balance (cash in, nothing owed against it) — which only a held stage_active payment produces: a fully-paid booking nets to 0, a deposit is negative, a refund nets back to 0. Because that cash has no sale leg, both existing guards (per-line price_paid and the DB hasPaidLine) missed it; this closes the gap with the same "refund this booking's payment first" message. Regression test: an attendee holding a payment leg with no sale is refused the no-quantity mark, and the line is left intact so the in-app refund still reaches.


Generated by Claude Code

Comment thread src/shared/db/checkout-stages.ts Outdated
Comment on lines +270 to +273
AND NOT EXISTS (
SELECT 1
FROM processed_payments AS payment
WHERE payment.payment_session_id = stage.payment_session_id

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep paid pending stages out of unpaid cleanup

When a paid staged checkout hits a retryable path before resolving the stage (for example a closed-listing refund attempt returns false, or a worker crashes and the unresolved reservation is later stale-cleaned), the processed_payments row is removed while the paid stage intentionally stays pending for the next delivery. This NOT EXISTS predicate then makes prunePendingCheckoutStages treat that paid-but-unresolved record as abandoned and delete its attendee after the retention window, so later webhook retries have no staged record to note, reconcile, or keep for the operator even though real money was captured.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accurate finding — and after weighing it with the owner, this one is a deliberate accepted edge, not a code change (documented in TODO.md, fa8b15d).

The reasoning: the money is never lost — it stays captured at the provider throughout, so the prune only removes our staged record, never the funds. And the failure that gets a stage into this state (a refund returning false, a ledger write failing, a crash) fires logError → ntfy + the admin activity log + Sentry on every delivery, and the provider re-delivers across its ~3-day retry window. So the operator is alerted repeatedly during the window and reconciles (manual refund / fix at the provider) well before the 7-day prune. Reacting to the failure is the fix, which moots the prune.

If it ever needs a hard fix, the TODO records the two paths: a durable "payment captured" mark on checkout_stages (stamped on the first paid delivery) skipped by prunePendingCheckoutStages, or recording the captured cash as a held payment leg on deferral so the record is kept and operator-actionable (the held-cash model, now guarded). The prune's NOT EXISTS processed_payments guard is unchanged.


Generated by Claude Code

Capture the locked design for the staged-checkout money model: validate fully
before the provider session, save the booking at quantity zero with the owed
ledger legs, add received-funds legs at payment, exclude pending-staged legs
from ledger sums (derived from the stage link), and delete legs when an
abandoned stage is pruned. Records the never-hold-a-seat policy and the
remaining review items.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 52937b8b3a

ℹ️ 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".

Comment on lines +250 to +252
const attendeeId = stage
? stage.attendeeId
: (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore staged ghost rows after listing deletes

Fresh evidence for the staged-row drift case: listingDeleteError is only a preflight read before performListingDelete, so a checkout can insert its pending stage after the guard passes and then have deleteListing remove its listing_attendees rows. In the deleted-listing refund path this branch reuses that now-empty staged attendee and ignores the datelessGhostBookings passed in, so the refund and note attach to a record with no booking rows and the operator loses the order the customer paid for; restore the signed ghost rows or make the delete/stage race impossible before resolving the stage.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5818fbb by closing the race at its source rather than repairing after it. deleteListing no longer cascades the listing_attendees rows of an attendee whose checkout is still pending, so a delete that slips past the preflight guard leaves the quantity-0 order intact — storeRefundedBooking then reuses a staged record that still has its rows, and the empty-staged-attendee state is now unreachable (so the ignored datelessGhostBookings no longer matter). A resolved (booked/failed) stage's rows are cascaded as before.

Regression test in test/shared/db/listings/delete.test.ts ("keeps a pending checkout's rows, but cascades a resolved one's") fails before the fix — the pending rows were deleted — and passes after.


Generated by Claude Code

claude added 4 commits July 13, 2026 18:03
Reorder createStagedCheckout so the order is fully validated BEFORE the
provider session is created: check the real quantities fit and every
listing is still on sale, and if not, stop and tell the customer up front
with a plain message instead of letting them pay and then refunding.

The staged rows themselves still claim nothing (quantity 0), so the
authoritative capacity claim continues to run at activation with the real
quantities — this new gate only stops a checkout that provably cannot
succeed. It is the single chokepoint for every new-booking entry point
(single booking, folded order, ticket page, QR), each of which already
checked availability separately; folding the check into the one shared
function keeps callers from ever handing a sold-out or off-sale order to
the provider.

Only new bookings flow through createStagedCheckout — a balance payment
settles an existing booking and goes straight to the provider — so the
dead balance-attendee handling is removed and the function always
validates and stages.

Adds tests that an already-overbooked listing and an off-sale listing are
both refused before the provider is reached (stubbed to throw), returning
the unavailable error and writing no stage.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu
The listing-delete guard blocks a delete while a pending checkout exists,
but it is a preflight read: a checkout that stages its booking in the
window between the guard and the delete would then have its listing_attendees
rows cascaded away. When that customer's payment lands, the deleted-listing
refund path reuses the now-empty staged attendee, so the refund and note
attach to a record with no booking rows — the operator loses the paid order.

Make the bad state impossible at the source: deleteListing no longer
cascades the rows of an attendee whose checkout is still pending, so a raced
delete leaves the quantity-0 order whole. The checkout then resolves
normally (its payment keeps the order and refunds) or the prune removes the
attendee outright. A resolved (booked/failed) stage's rows are cascaded like
any other booking.

Adds a regression test that a pending checkout's rows survive a listing
delete while a booked one's are removed (fails before the fix: the pending
rows were deleted).
Keep all ledger legs at activation rather than posting owed legs at staging:
the ledger posts an event group as one immutable set, so owed-at-staging plus
received-at-activation would need two event groups and careful handling of
the sale-scoped per-row projection — for no change in any total, since a
pending stage (no legs, quantity 0) is already excluded from every sum and
activation is already atomic. plan.md records the decision and why the
owed-at-staging slices are dropped.

Also document the standing no-seat-holding policy in AGENTS.md: a checkout
never reserves capacity (quantity-0 staging; first payment wins; a loser is
refunded), because holding seats invites botting.
The activation flip to 'booked' was an unconditional UPDATE by session id: if
the stage were no longer this attendee's pending stage — a concurrent
resolution, or a session/attendee mismatch — it would silently succeed and
book a stage that was already resolved.

Make it a compare-and-set: only a row that is still this attendee's PENDING
stage may flip to booked, and exactly one must. The row-level pre-check and
the fail-closed guard mean the stage IS pending here, so a miss is an
impossible state — throw to roll the whole activation back rather than book a
stage that isn't ours, and let the webhook redelivery re-resolve it (a
resolved stage reads as already-handled at the preflight).

Adds a test that activation throws and rolls back (rows stay quantity 0) when
the stage was resolved out from under it, and folds the shared throw+rollback
assertion into a helper.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d73c4f05e9

ℹ️ 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".

Comment on lines +19 to +23
sql: `DELETE FROM listing_attendees
WHERE listing_id = ?
AND attendee_id NOT IN (
SELECT attendee_id FROM checkout_stages WHERE state = 'pending'
)`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep deleted staged listings visible to operators

Fresh evidence for the earlier staged-row drift case: the preserved row still cannot render because getRenderListings builds the attendee editor/booking table from getAllListings() and only includes active or inactive rows that still exist in listings, while this batch deletes the listing row later. When a stage is inserted after listingDeleteError but before this delete, the payment refund keeps a listing_attendees row whose listing no longer resolves, so attendeeBookingsFromLines drops it and the operator still cannot see what the customer bought; either keep a renderable ghost listing/line or make the delete race impossible.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data loss is fixed — 5818fbb preserves the pending-staged rows, so the refund, the ledger money round-trip, and the system note all attach to a record that still holds the order. What remains is the bookings table dropping a line whose listing was deleted (attendeeBookingsFromLines resolves each line's listing from getAllListings() and skips a missing one). That's a pre-existing render limitation shared with the no-stage datelessGhostBookings refund path — both create rows for a deleted listing — and the operator still sees the situation via the system note and the payment/ledger panel.

Closing the race can't remove deleted-listing ghosts anyway: when a listing is deleted mid-payment the no-stage path creates one regardless, so the durable fix is a renderable "deleted listing" placeholder line (read-only — a deleted listing can't be re-booked). That's an operator-UX call I've deliberately folded into the admin-lifecycle work (review item #8) so the placeholder is designed once across every deleted/pending case rather than piecemeal. Recorded in TODO.md (756503b).


Generated by Claude Code

claude added 2 commits July 13, 2026 19:28
A booking whose listing was deleted is dropped from the operator attendee
table (attendeeBookingsFromLines resolves each line's listing from
getAllListings and skips a missing one). This is inherent to any
deleted-listing ghost — the no-stage datelessGhostBookings refund path and,
after the delete/stage race fix, a raced-delete staged order. The data is
intact; only the table line is hidden. Records the "renderable deleted-listing
placeholder line" follow-up alongside the admin-lifecycle work so the look is
decided once across every deleted/pending case.
Two coupled fixes to the terminal staged-refund paths, both about a failed
ledger post:

- Stamp the payment reference only AFTER the money is recorded. Both
  recordHeldStagedMoney and storeRefundedBooking stamped the provider payment
  id onto the staged attendee before posting the held-payment leg. If that post
  failed, the throw left a still-pending record carrying a refundable reference,
  so the Actions tab would offer an in-app refund for a charge the ledger had
  not recorded — refund it there and the retry records held cash out of sync.
  Posting first means a failed post throws before any reference is stamped.

- Honour a failed placeholder post on a staged order. storeRefundedBooking
  ignored recordPlaceholderRefund's {posted:false}, so a staged keep-and-refund
  whose ledger round-trip could not be written still wrote the note, resolved
  the stage, and went terminal with the money missing from the books — later
  deliveries then replayed the resolved stage and could not repair it. It now
  throws on a failed post when a stage can absorb the retry, leaving the stage
  pending for the redelivery (the settled refund reads back as refunded, so it
  never refunds twice). The no-stage path can't retry without minting a
  duplicate placeholder, so it still proceeds — the miss stays logged.

Tests: the kept-and-refund placeholder path throws and stays pending on a
blocked post (then resolves on retry), and neither terminal path leaves a
payment reference stamped after a failed post.
claude added 3 commits July 13, 2026 21:45
The owner's call on the last Codex finding: a paid-but-stuck staged checkout
(processing failing past the provider's ~3-day webhook-retry window) can be
pruned at 7 days, deleting our record of a payment still captured at the
provider. Left as-is because the money is never lost (recoverable at the
provider) and every failing delivery alerts the operator (ntfy + admin activity
log + Sentry) across the retry window, so they reconcile well before the prune.
Records the accepted trade and the two future hard-fix paths (a durable capture
mark, or recording held cash on deferral) so the decision is visible and not
re-litigated.
Records everything shipped (items 1, 6, 3-decision, the compare-and-set flip,
and all eight Codex findings including the accepted paid-stage-prune edge) and
lays out the remaining order of work: #8 admin lifecycle next, then #9 provider
lifecycle, then #10 cleanup, with #5/#7 deliberately parked in TODO.md.
Item 8 of the staged-checkout review, first half: a record whose checkout is
still pending is locked server-side, but the UI still offered every control
and read like an ordinary zero-quantity booking. Now:

- The attendee page hides the Edit, Logistics, and Actions tabs (and the
  send-email button) while a checkout is pending — every mutation they lead to
  is blocked anyway, and a hidden tab's URL is a 404. A banner alert explains:
  "Checkout pending: the customer may still be paying. This record is locked
  until the payment finishes or expires." The flag loads once on the shared
  page entity.
- An edit submission that raced the checkout redirects to the always-visible
  overview with the same refusal as a flash (the hidden Edit tab could no
  longer re-render the form refusal in place), replacing the in-save guard.
- Every attendee row now carries a `pending_checkout` flag, projected per row
  from checkout_stages in the one shared attendee SELECT — so the attendee
  tables' status and ticket cells say "Payment in progress" instead of the
  "No quantity" wording that invites an edit.
- CSV exports (attendees + calendar) gain a "Checkout pending" column, emitted
  only when some exported row is mid-payment, so a pending row never reads as
  a plain zero-quantity booking.

Tests: tabs hidden + 404 while pending and restored once resolved, the banner
alert, the raced-edit redirect, the table wording end-to-end through the
browsing page, and the conditional CSV column.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 880ac8eed0

ℹ️ 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".

// finishes, exactly like the Edit tab.
if (await hasPendingCheckout(attendeeId)) {
return redirect(
attendeePage.path(attendeeId, "logistics"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Redirect pending logistics saves to a visible tab

When a Logistics POST races with a newly staged checkout, this redirects back to the Logistics tab, but attendee-page.ts now hides that tab while pendingCheckout is true and the hidden tab URL 404s. In that scenario the operator lands on a missing page instead of seeing the pending-checkout flash/banner; the Edit handler already redirects to the overview for this exact reason, so this guard should target the always-visible attendee overview too.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4c8aeff. The Logistics mid-payment refusal now redirects to the always-visible overview (attendeePage.path(id, "")), exactly like the Edit handler — the flash carries the "mid-payment" refusal and the banner explains the locked state. Regression test updated: the redirect lands on the overview, never the hidden /logistics URL.


Generated by Claude Code

claude added 3 commits July 13, 2026 23:16
Item 8 of the staged-checkout review, second half:

- A stored note can carry a markdown link to the ledger (the refund notes
  do), but the ledger pages are owner-only — every other admin saw a link
  that only 404s. Note bodies now demote an /admin/ledger link to its plain
  text for non-owners (new withoutLinksTo markdown helper), threaded through
  the attendee banner, the notes summaries above the attendee/listing lists,
  and the delete-note page.
- A booking row can outlive its listing (a delete racing a mid-payment
  checkout keeps the staged rows), but the read-only bookings table silently
  dropped such a line — the operator couldn't see what the customer paid
  for. It now shows a plain "Deleted listing" placeholder row instead, with
  no link (the listing page would 404) and no "(Inactive)" note. The line
  builder's untrue "deleting a listing deletes its rows" non-null assertion
  becomes an honest null.
- The Logistics mid-payment refusal now redirects to the always-visible
  overview, like the Edit one — its old target (the Logistics tab) is hidden
  while pending, so the flash landed on a 404 (raised by Codex).

Tests: owner keeps the note's ledger link and a non-owner gets plain text
(unit + banner), the placeholder line renders without a dead link (unit +
end-to-end through a raced-delete staged record), and the logistics refusal
lands on the overview.
Item 9 of the staged-checkout review. Webhook setup deleted only the endpoint
id recorded in our settings before recreating — an endpoint on the same url
whose id we lost (a database restore, a re-setup that didn't save) was never
found, so it lived on at Stripe and failed signature verification against the
new secret on every delivery, forever. Setup now lists the account's endpoints
and removes every one pointing at OUR webhook url (plus the recorded id, which
may point at an old url after a domain change) before creating the fresh
endpoint. Endpoints on other urls are never touched. Listing is best-effort:
if it fails, setup still deletes the recorded endpoint and creates the new one.

The two places that list webhook endpoints (this reconcile and the settings
health check) now share one listWebhookEndpoints helper.

Also locks the rest of item 9 with a direct test and documentation:
- SumUp EXPIRED maps to the terminal "failed" status (new provider test), so a
  lapsed checkout takes the same discard-and-cancel-page path as a declined
  card.
- The cancel path's local-discard-then-remote-expire order is deliberate (the
  discard's claim guard is what makes closing the remote session safe) and now
  documented in plan.md, with every gap landing on the designed no-stage
  fresh-booking path.

Test: setup against a mocked endpoints API removes the recorded endpoint and
the same-url stray, keeps the other-url endpoint, and returns the fresh secret
(fails before the fix — only the recorded endpoint was removed).
Item 10 of the staged-checkout review. Three places each kept their own list
of the tables that hang off an attendee: the single-attendee delete, the
orphaned-attendee purge (with a special case for service_costs' different key
column), and the pending-checkout discard (importing the raw
DEPENDENT_ROW_TARGETS list to build its own statements). Three parallel
implementations of one operation is exactly the drift the review flagged.

All three now build from one mechanism in delete.ts: attendeePurgeStatements
(internal — the raw table list is no longer exported) run through
runAttendeePurge, which reports the final statement's affected rows. The
discard's one genuine difference is a named option (stagesLast: its id-select
reads checkout_stages, so those rows must outlive every other delete); the
orphan purge gains service_costs parity through the shared field mapping
instead of a hand-written special case. A new dependent table added to the
one list is cleaned by every purge path automatically.

Behaviour is unchanged — the existing delete, orphan-purge, discard, and
prune suites all pass as-is, and plan.md now records items 8-10 as shipped.
@stefan-burke stefan-burke changed the title Stage each paid order before payment, and harden the flow Stage each paid order before payment, and harden the whole flow Jul 13, 2026
CI failed on "generates different slugs on multiple calls": 20 random
5-character slugs (~1.15M combinations) asserted zero collisions, which is a
birthday problem — a real collision fires about once per 6,000 runs, and one
just did. Unrelated to this branch; fixed in passing per the good-citizen rule.

The test now drives generateSlug with two disjoint pinned Math.random
sequences and asserts the outputs differ — deterministic, and it still catches
a cached or constant slug that ignores the randomness (the format and
Fisher-Yates tests beside it cover the rest of the contract).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

visible: (_entity, session) => session.adminLevel === "owner",

P2 Badge Hide ledger writes while checkout is pending

This leaves the owner-only Ledger tab visible for a pending staged checkout even though that tab embeds AccountStatementActions, which offers owner-entered ledger entries for attendee accounts. In the inspected attendee page flow, an owner can add a manual attendee charge/payment while the customer is still paying; when the provider payment later activates, the signed checkout posts its own sale/payment legs but the manual entry survives, so a fully paid ticket can unexpectedly show a balance or credit despite the mid-payment freeze applied to the Edit/Logistics/Actions tabs. Gate this tab's write controls, or the tab itself, on the same notMidPayment condition.

ℹ️ 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".

// normally resolves. It is null for a DELETED listing: a delete racing a
// mid-payment checkout keeps the staged rows, and the read-only bookings
// table shows those as a "Deleted listing" placeholder.
listing: listingsById.get(existing.booking.listing_id) ?? null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep deleted rows out of the edit form

When a listing is deleted during a pending checkout, this now builds an edit-form line with listing: null, but the Edit tab still renders every line through ListingRow, which immediately dereferences line.listing! and reads listing.listing_type. After the payment path resolves the stage, pendingCheckout is false and the operator can open the Edit tab for the kept refund record, causing the attendee page to throw instead of letting them view or repair the record; the null placeholder should be limited to read-only booking summaries or the editor must skip/render it safely.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ecb5f81. The editor now renders a listing: null line as a locked row instead of throwing: a "Deleted listing" placeholder, the line's hidden identity fields, and a hidden no-quantity tick — no editable controls. Locked (not skipped) because the save deletes any row its form omits, so filtering the line out would silently drop the kept record's booking on the next save. The model retains the row (isNoQuantityLine now keeps a stored row whose listing is gone), the desired line re-states the row's own stored date range (so the atomic edit sees an unchanged preserve), and validation refuses a hand-crafted submission that un-ticks the lock. Regression tests: the edit form renders 200 with the locked row, an unrelated save keeps the quantity-0 row, the un-lock attempt 400s with the row intact, plus unit tests on retention/range/validation — all fail before the fix.


Generated by Claude Code

claude added 2 commits July 14, 2026 00:17
Two Codex findings on PR #1802, both from the delete/mid-payment race
that keeps a booking row alive after its listing is gone:

- The Ledger tab now hides (and its URL 404s) while a checkout is
  pending, like the other write tabs — it embeds manual charge/payment
  forms, and a manual leg posted mid-payment would combine with
  activation's own legs into a surprise balance.

- The Edit tab no longer throws on a kept row whose listing was
  deleted. The row renders locked: a 'Deleted listing' placeholder, a
  hidden no-quantity tick, and no controls, so every save submits it
  unchanged (the save deletes any row its form leaves out). Validation
  refuses a hand-crafted submission that un-ticks the lock, and the
  saved line keeps the row's own stored date range instead of the
  shared range.

Regression tests fail before each fix: the hides-write-tabs test now
covers /ledger, and new edit/unit tests cover the locked row's render,
retention, range preservation, and the un-lock refusal.

Copy link
Copy Markdown
Member Author

On Codex's review-body finding ("Hide ledger writes while checkout is pending", attendee-page.ts:257): fixed in ecb5f81. The Ledger tab's visible is now ANDed with the same notMidPayment gate as the Edit/Logistics/Actions tabs, so it hides while a checkout is pending and its URL 404s (visible IS the authorization on the entity page). That closes the window where an owner could post a manual charge/payment mid-payment and have it combine with activation's own legs into a surprise balance — and a pending stage has zero ledger legs, so the tab had nothing to show anyway. The hides-write-tabs regression test now also asserts the /ledger link is absent and the URL 404s while pending, and returns once the stage resolves; it fails before the fix.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ecb5f81154

ℹ️ 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".

Comment thread src/shared/listings-actions.ts Outdated
Comment on lines +488 to +490
(await listingHasPendingCheckout(listingId))
? t("admin.listing.pending_checkout_delete")
: deleteOrphanedAddOnError(listingId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Block listing deletes while conflict cash is held

When a staged checkout reaches the stage_active path, its stage is marked failed after recording a provider payment leg with no sale, so listingHasPendingCheckout no longer matches. The listing delete flow then falls through here and deleteListing removes that attendee's booking row because it only preserves state = 'pending' rows; after that canRefundAttendee hides the refund action because there is no active booking line, leaving the held charge without the in-app refund path the conflict note tells the operator to use. Please also reject deleting a listing while any attendee on it holds unreturned cash.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in db71893. listingDeleteError now also refuses the delete while any attendee booked on the listing holds unreturned conflict cash — new listingHoldsUnreturnedCash reads the listing's attendee ids and runs them through the same primary-pinned attendeeIdsHoldingUnreturnedCash batch guard the no-quantity/delete/merge gates use, so every write gate that consults held cash stays one mechanism. The refusal message tells the operator to refund first. Regression test: a listing whose attendee carries a held payment (no sale) is refused deletion and survives, keeping the booking line the in-app refund needs — fails before the fix.


Generated by Claude Code

Comment on lines +261 to +262
visible: (entity, session) =>
session.adminLevel === "owner" && notMidPayment(entity),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Block standalone ledger writes during checkout

This hides the attendee Ledger tab while a stage is pending, but the standalone owner ledger routes are still writable: checked src/features/admin/ledger/entries.ts, and POST /admin/ledger/:type/:ref/add loads an attendee account and calls postManualLedgerEntry without checking hasPendingCheckout. An owner with /admin/ledger/attendee/<id>/add can therefore post a manual payment or charge while the provider checkout is still in flight; when activation later posts the checkout's own sale/payment legs, the account can show a surprise balance. Please apply the same pending-checkout guard to the standalone attendee ledger write route.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in db71893. POST /admin/ledger/:type/:ref/add now refuses an attendee-account entry while that attendee's checkout is pending, with the same "mid-payment" refusal flash the other write guards use. The guard lives in the write handler itself — the one route behind both the (already hidden) Ledger tab's form and the standalone statement page — so no surface can post a manual leg that would combine with activation's own sale/payment legs into a surprise balance. Regression test: a manual payment against a pending staged attendee is refused and no transfer is written — fails before the fix.


Generated by Claude Code

…d money

Two more Codex findings on the same theme:

- The standalone ledger add route (POST /admin/ledger/attendee/:id/add)
  now refuses a manual charge or payment while the attendee's checkout
  is pending — the activation posts its own sale/payment legs when the
  payment lands, so a manual entry added mid-payment would combine with
  them into a surprise balance. This is the write route behind both the
  (already hidden) Ledger tab and the standalone statement page, so the
  freeze is enforced where the money moves.

- Deleting a listing is now refused while any attendee booked on it
  holds unreturned conflict cash. The delete cascades the booking rows,
  and the in-app refund needs an active booking line, so the delete
  would strand the held charge with no refund path. New
  listingHoldsUnreturnedCash reuses the primary-pinned held-cash batch
  guard over the listing's attendees.

Both regression tests fail before their fix: the mid-payment manual
entry is refused with no transfer written, and the held-cash listing
delete is refused with the listing intact.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db718939a7

ℹ️ 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".

},
],
slug: "actions",
visible: notMidPayment,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Hide actions when the home listing is gone

When a checkout races with listing deletion, this PR now keeps the attendee row with a listing_attendees.listing_id whose listings row has been deleted. After the stage resolves, pendingCheckout is false, so the Actions tab is shown here; however the attendee-scoped action routes still load the home listing with getListingWithCount(attendee.listing_id) and return null when it is missing (src/features/admin/attendees-route-helpers.ts:85-87). That leaves delete/resend/refund action links visible on the retained deleted-listing record even though those targets 404, so either gate the actions on a renderable home listing or make the action loader handle deleted-listing records.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0ffd541 with the first option — gate the actions on a renderable home listing. LoadedAttendee gains homeListingExists (loaded via getListingWithCount(attendee.listing_id) !== null, the exact condition loadAttendeeWithHomeListing enforces), and the Actions tab's visible is now notMidPayment(entity) && entity.homeListingExists. So a resolved deleted-listing record no longer renders delete/resend/send-text links that 404 (refund was already gated on canRefund, which is false for a quantity-0 record) — the tab hides just as it does while a checkout is pending. Making the action loader fully handle deleted-listing records (so delete/refund actually work on them) is the larger "design the deleted-listing operator UX once" item still tracked in TODO.md; this closes the dead-link half now. Regression test: after the stage resolves on a raced-delete record, the /actions link is absent and GET …/actions 404s while the Edit tab is back — fails before the fix.


Generated by Claude Code

Comment on lines +439 to +440
parsed.lines.some(isNoQuantityLine) &&
(await attendeeHoldsUnreturnedCash(attendeeId))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Only block no-quantity saves that remove the refund path

Fresh evidence: this new held-cash guard rejects any submitted form that contains a no-quantity line, not just edits that remove the last line the in-app refund can use. A stage_active record can have one staged row flipped live while another signed row is still quantity 0; when the live row is the attendee's home line, the refund route still has an active booking, but any later save submits the remaining no-quantity row and is refused here. That blocks the operator from fixing the quantities the conflict note asks them to check unless they refund first, even when the edit would leave the refund path intact.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0ffd541. The guard no longer fires on "any no-quantity line present" — it now blocks only a save that would leave the home listing with no active booking line while held cash exists (keepsHomeRefundLine = parsed.lines.some(line => line.listingId === attendee.listing_id && (line.quantity ?? 0) >= 1)). The refund route is home-scoped (canRefundAttendee → hasActiveBookingLine(id, attendee.listing_id)), so a save that keeps an active home line leaves the refund path intact and is now allowed — the operator can fix the other quantities the conflict note asks about. As a bonus it also catches a case the old some(isNoQuantityLine) check missed: removing the home line by zeroing it un-ticked (not marking it no-quantity) also strands the cash, and is now blocked. The paid-line and held-cash refusals share one guard/message now. Regression tests: an edit that keeps the home line active while marking another line no-quantity now saves (fails before the fix), and marking the home line no-quantity while another stays active is still blocked.


Generated by Claude Code

// success, so a retry never refunds twice). A crash between the ledger post
// and this line is healed by the next delivery's orphaned-ledger answer,
// which resolves the leftover pending stage (resolvePendingStage).
if (stage) await markCheckoutStage(session.id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep failed staged refunds retryable

When tryRefund returns false for a staged stored-refund path, recordPlaceholderRefund(..., refunded=false) still posts the provider payment leg, and this line then marks the stage failed. processPaymentSession releases the reservation for refunded === false so the next delivery can retry the refund, but that retry now hits the ledger preflight: the payment leg makes the session look already handled, the stage is no longer pending, and tryRefund is never called again. Leave the staged record retryable until the provider refund succeeds, or avoid writing the terminal held-money leg before resolving the failed refund.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0ffd541, taking the second option you suggested — don't write the terminal held-money leg before the refund settles. storeRefundedBooking now splits by whether the order was staged: when there IS a stage and tryRefund returned false, it returns early with refunded: false and posts NO ledger legs, leaving the stage pending. That releases the reservation and returns 503, so the provider redelivers; the retry finds the ledger unrecorded (bookingLedgerDisposition → unrecorded) and the stage still pending, so it re-runs the whole path and re-attempts tryRefund until it settles — the same retryable design as failStagedValidation. The no-stage path is unchanged and stays terminal (a retry there would mint a duplicate placeholder beside the one already created). Regression test: a staged capacity-loss order whose refund fails stays pending with no legs and no stamped reference, then resolves with the full payment+refund_cash round-trip once the refund succeeds on retry — fails before the fix (which resolved the stage and posted the payment leg).


Generated by Claude Code

claude and others added 5 commits July 14, 2026 08:29
…ead action links

Three more Codex findings on the staged-checkout flow:

- A staged order whose provider refund FAILED now stays retryable
  instead of going terminal. Before, storeRefundedBooking posted the
  held-money payment leg and resolved the stage even when the refund
  didn't settle; the next delivery then read the ledger as 'already
  handled' and never re-attempted the refund, stranding the money until
  an operator refunded by hand. It now leaves the stage pending with no
  ledger legs and reports the refund as unsettled, so the provider
  redelivers and the refund is retried until it lands — mirroring the
  closed-listing path. The no-stage path stays terminal (a retry there
  would mint a duplicate placeholder).

- The held-cash no-quantity guard no longer over-blocks. It refused any
  edit that touched a no-quantity line while conflict cash was held, so
  an operator couldn't fix the other quantities the conflict note asks
  them to check. It now blocks only a save that removes the active home
  line the in-app refund needs, leaving edits that keep it intact.

- The attendee Actions tab now hides when the record's listing was
  deleted mid-payment. Every attendee-scoped action route loads the home
  listing and 404s once it is gone, so those delete/resend/refund links
  only broke on click; the tab now hides like it does while a checkout
  is pending.

Each ships with a regression test that fails before its fix. The
staged-checkout test file crossed the 1000-line ceiling, so its shared
setup moved to server-payment-staging-helpers.ts and the recovery-focused
tests to their own file.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 31a4910e64

ℹ️ 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".

Comment on lines +119 to +123
SET quantity = (
SELECT CAST(json_extract(expected.value, '$.quantity') AS INTEGER)
FROM expected
WHERE ${expectedLineMatch("booking", "expected")}
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refresh staged booking dates on activation

If a daily listing’s duration changes while a staged checkout is pending, activation checks capacity using the newly recomputed bookings span but this update only flips the old staged rows from quantity 0 to the paid quantity. The row’s existing start_at/end_at from checkout creation are left in place, and the match above only keys on start_at, so shortening a listing mid-payment can activate an old longer row after checking capacity only for the new shorter span. Include the expected end date in the stage match or rewrite the date fields together with the quantity.

Useful? React with 👍 / 👎.

// failed, the money round-trip recorded, and its record noted, or the
// pending stage holds the PII unprunable for as long as the payment row
// lives.
return failStagedValidation(session, intent, signedListingId, validated);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Mark the stage refunding before validation refunds

Fresh evidence: the known-listing failure refund still happens inside validateAllItems before this staged handoff can call beginCheckoutStageRefund. If a closed/deactivated staged checkout is refunded and the worker crashes before reaching this line, the stage remains pending with no ledger record; after the stale reservation is reclaimed, a redelivery can re-run validation and, if the listing has reopened, activate a live ticket for money that has already been returned. Move the provider refund behind the stage's refunding transition or return a refund-needed result that this staged path performs after that transition.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a5eeab0c9

ℹ️ 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".

* "payment in progress" instead of the "No quantity" sentinel wording. */
const PENDING_CHECKOUT_SQL =
"(SELECT EXISTS(SELECT 1 FROM checkout_stages AS stage" +
` WHERE stage.attendee_id = attendee.id AND stage.state = 'pending'))` +

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include refunding stages in checkout status

When a staged refund has begun (beginCheckoutStageRefund sets the stage to refunding after a closed/deactivated mid-payment checkout), all mutation guards treat it as open via hasPendingCheckout, but this projection returns pending_checkout = 0. The attendees/dashboard tables and CSV therefore show the quantity-0 row as ordinary “No quantity” or omit the checkout-pending column while the detail page is still locked, hiding a payment/refund still in flight; use the shared open-state predicate here too.

Useful? React with 👍 / 👎.

Comment on lines +70 to +71
`DELETE FROM checkout_stages
WHERE state IN ('booked', 'failed') AND created_at < ?`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add a bounded path for refunding stages

If a staged refund enters refunding and refund attempts keep failing until provider redeliveries stop, the row is no longer eligible for the pending-stage purge and this resolved-stage purge only removes booked/failed. Because admin mutation guards include refunding as an open stage, that attendee remains locked and unprunable indefinitely with no scheduled reconciliation path; old refunding rows need a terminal/operator-visible recovery path instead of being omitted from cleanup.

Useful? React with 👍 / 👎.

Comment on lines +99 to +103
const available = await checkBatchAvailability(
bookings.map(bookingCapacityFields),
intent.date,
);
return available ? null : { error: t("public.checkout_unavailable") };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Check registration cutoff before creating checkout

This new pre-provider refusal only asks the capacity checker, which enforces active listings and space but not registration_closes_at. If registration closes after the form context is prepared (or on the direct QR checkout path) but before this call, available can still be true and the customer reaches the card page for an order that validateAllItems will immediately refund after payment; include the same registration-closed check here so closed orders are refused before money moves.

Useful? React with 👍 / 👎.

@stefan-burke

Copy link
Copy Markdown
Member Author

Closing — superseded by #1853 (split/staged-checkout-runtime), which is now the canonical staged-checkout PR.

When this PR was filed it carried two layers at once:

  1. The staged-checkout runtime (write bookings at quantity zero before payment, activate atomically on payment success, shared close/refund across Stripe/Square/SumUp).
  2. An integrity layer on top: backups that survive in-flight payments, payment safety across site upgrades/moves, mid-payment record locking, deleted-listing survival, stray Stripe webhook endpoint cleanup.

We've decided against (2). The integrity layer added real complexity for cases we don't need to support — the running version doesn't need to preserve open payments across backups or site upgrades. Removing that defensive scaffolding rather than maintaining it.

That leaves the runtime as the thing to ship, and it has been split off to #1853. That branch is actively passing precommit, has absorbed the latest main, and addresses Codex review. It carries the same deferred-architecture design notes (STAGED_TODOS.md, STAGED_DESIGN.md, SCHEDULED_DESIGN.md, COMPLETION_DESIGN.md) as documentation only.

One non-integrity piece unique to this PR is worth a follow-up if it turns out to still be needed:

  • src/shared/stripe-webhook-reconcile.ts removed any stray webhook endpoints already pointing at this site on Stripe setup (left by a lost setup or database restore, which would fail signature checks forever). If Finish and recover paid checkouts safely #1853 still creates stray endpoints in those scenarios, re-add this as a small standalone fix.

The 170 unique files on this branch remain recoverable in git history. The branch itself is intentionally left in place (not deleted) for the same reason.

@stefan-burke
stefan-burke deleted the claude/branch-review-comparison-pcwbvd branch August 24, 2026 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants