Keep existing payments working when new sales are off - #2020
Merged
Conversation
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (111)
📝 WalkthroughWalkthroughThis PR separates provider resolution for new checkouts from provider resolution for existing payments. It adds a ChangesPayment provider safety
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant AdminSettingsPage
participant SettingsGeneralRoute
participant Settings
participant ExistingPaymentProvider
AdminSettingsPage->>SettingsGeneralRoute: POST /admin/settings/payment-provider-recovery
SettingsGeneralRoute->>ExistingPaymentProvider: existingPaymentProviderState()
ExistingPaymentProvider-->>SettingsGeneralRoute: recovery choices
SettingsGeneralRoute->>Settings: withCurrentTask(recoverPaymentProvider)
Settings-->>SettingsGeneralRoute: updated provider state
SettingsGeneralRoute-->>AdminSettingsPage: redirect with success
sequenceDiagram
participant Webhook
participant GetPaymentProviderForExistingPayments
participant ExistingPaymentProvider
participant Settings
participant PaymentProvider
Webhook->>GetPaymentProviderForExistingPayments: resolve provider
GetPaymentProviderForExistingPayments->>ExistingPaymentProvider: existingPaymentProviderType()
ExistingPaymentProvider->>Settings: read active/remembered provider
ExistingPaymentProvider->>PaymentProvider: infer from stored credentials
ExistingPaymentProvider-->>GetPaymentProviderForExistingPayments: resolved provider
GetPaymentProviderForExistingPayments-->>Webhook: provider or null
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 `@src/features/api/payment-processing/refunds.ts`:
- Around line 40-50: Define and export a shared ExistingPaymentProviderResult
type from `#shared/payments.ts`, then update getPaymentProviderOrLog in
src/features/api/payment-processing/refunds.ts#L40-L50 to use it instead of the
inline Awaited<ReturnType<...>> expression. Also update authenticateWebhook in
src/features/api/webhooks.ts#L321-L329 to use
NonNullable<ExistingPaymentProviderResult>, preserving its existing non-nullable
return contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: beec80eb-fa11-41a1-a2b5-fac7fdeca1e3
📒 Files selected for processing (18)
TODO.mddocs/payment-aggregate-acceptance.mdscripts/mutation/equivalent-mutants/shared-m-z.txtsrc/features/admin/require-provider.tssrc/features/api/payment-processing/classify.tssrc/features/api/payment-processing/refunds.tssrc/features/api/webhooks.tssrc/features/settings-bundles.tssrc/shared/db/settings.tssrc/shared/payments.tssrc/shared/settings/keys.tssrc/shared/settings/registry.tstest/integration/server/payments/sales-off-safety.test.tstest/integration/server/webhooks/refund-skip-conditions.test.tstest/shared/db/settings/public-api.test.tstest/shared/payments.test.tstest/shared/settings/keys.test.tstest/shared/settings/registry.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e4f452de1
ℹ️ 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".
Switching new sales off (saving the payment provider as "none") used to strand every payment already captured: refunds, replayed callbacks, redirect completion, the cancel page, and operator refunds all resolved the provider through the new-sales gate, so they broke the moment a buyer could no longer start a checkout -- even though the provider's credentials stay stored. Separate the two questions. New sales keep using getActivePaymentProvider / isPaymentsEnabled. Refunds, provider reconciliation, replayed callbacks, and completion of already-started payment work use a new resolver, getPaymentProviderForExistingPayments, which falls back to the last provider the operator activated when new sales are off. That provider is remembered in a new last_active_payment_provider setting, captured when a provider is switched on and again when new sales are switched off (a second "none" save keeps it). The seven accepted safety rules the payment aggregate must satisfy -- including the owner-review, queued owner email, and aggregate-activation behaviour not yet on main -- are recorded as acceptance constraints in docs/payment-aggregate-acceptance.md, not built ahead of their time. Regression tests cover each path: a captured payment refunded while sales are off, an in-flight payment completing and issuing the ticket, a handled failure replaying on a redelivered callback, and an operator refunding an existing paid booking -- all written to fail before the fix and pass after.
stefan-burke
force-pushed
the
payment-aggregate/01-safety-behavior
branch
from
August 1, 2026 00:49
2e4f452 to
dc137e9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc137e955d
ℹ️ 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".
CodeRabbit (CHANGES_REQUESTED): export a named ExistingPaymentProvider type from #shared/payments.ts and use it in place of the duplicated Awaited<ReturnType<typeof getPaymentProviderForExistingPayments>> expression in getPaymentProviderOrLog and authenticateWebhook. Codex P2: persist PAYMENT_PROVIDER and LAST_ACTIVE_PAYMENT_PROVIDER in one writeRawBatch transaction (mirroring the existing Stripe activation path), so a failure between the two writes cannot leave new sales enabled against a stale remembered provider. The snapshot mirrors the committed values only after the batch succeeds. A second 'none' save keeps the remembered provider. Codex P1 (per-charge provider) and P1 (pre-existing none sites): recorded as known gaps in docs/payment-aggregate-acceptance.md and TODO.md rather than implemented here — per-charge provider tracking is the reference a charge lives in and is future aggregate work; a site already on 'none' with no recorded last-active provider is not regressed by this PR and recovers via a one-click provider re-select.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc4a6dc84b
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
TODO.md (2)
34-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Markdown reference to PR
#1995.Line 35 starts with
#1995.. This triggers MD018 and renders as a malformed heading. Keep the reference in the sentence, such asshipped on PR#1995..🤖 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 `@TODO.md` around lines 34 - 35, Update the Finding 1 sentence in TODO.md so the PR reference remains inline, ensuring it uses “PR `#1995`” rather than starting the line with “#1995.” and triggering MD018.Source: Linters/SAST tools
25-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace local scan references with durable references.
The TODO records an absolute
/home/user/...path and local worktree names. These values are not available to other contributors and will become stale. Use stable ticket, PR, or repository-relative references.🤖 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 `@TODO.md` around lines 25 - 32, Update the TODO entry describing Findings 2 and 4 to remove the absolute local scan path and local worktree names, replacing them with durable ticket, pull-request, or repository-relative references while preserving the findings’ descriptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/db/settings.ts`:
- Around line 464-480: Update setPaymentProviderNone so keepLastActive is
derived from the current database state or the provider transition is
serialized, preventing a concurrent activation from persisting a stale
LAST_ACTIVE_PAYMENT_PROVIDER alongside payment_provider = "none". Preserve the
behavior of retaining the last active provider on repeated disable operations,
and add a regression test covering the disable/activation interleaving.
In `@test/integration/server/payments/sales-off-safety.test.ts`:
- Around line 93-125: Replace the manual try/finally disposal at
test/integration/server/payments/sales-off-safety.test.ts lines 93-125 and lines
195-207 with using declarations for the stub results: use using mockVerify for
the verifyWebhookSignature stub and using mockRefund for the refund stub, then
keep each test body directly after its declaration and remove the corresponding
restore calls and try/finally wrappers.
In `@TODO.md`:
- Around line 492-494: The existing-payment guarantee is too broad because
fallback does not track the provider that captured each charge. In TODO.md lines
492-494 and docs/payment-aggregate-acceptance.md lines 21-25, replace the
unconditional claim with wording that guarantees recovery only for payments
captured by the remembered provider, while explicitly retaining the per-charge
provider identity gap.
- Around line 513-516: Update the payment persistence and dispatch flow so each
payment charge retains its capture-time provider using the existing
payment_charges.provider and reference_index fields, rather than storing
provider identity only on processed_payments. Backfill legacy charges only when
capture-time evidence identifies one unambiguous provider; mark unresolved
charges non-dispatchable and expose them for operator review, then dispatch
existing-payment work from each charge’s provider instead of a global fallback.
---
Outside diff comments:
In `@TODO.md`:
- Around line 34-35: Update the Finding 1 sentence in TODO.md so the PR
reference remains inline, ensuring it uses “PR `#1995`” rather than starting the
line with “#1995.” and triggering MD018.
- Around line 25-32: Update the TODO entry describing Findings 2 and 4 to remove
the absolute local scan path and local worktree names, replacing them with
durable ticket, pull-request, or repository-relative references while preserving
the findings’ descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee66fda4-586e-4c45-8d73-c697b958b976
📒 Files selected for processing (18)
TODO.mddocs/payment-aggregate-acceptance.mdscripts/mutation/equivalent-mutants/shared-m-z.txtsrc/features/admin/require-provider.tssrc/features/api/payment-processing/classify.tssrc/features/api/payment-processing/refunds.tssrc/features/api/webhooks.tssrc/features/settings-bundles.tssrc/shared/db/settings.tssrc/shared/payments.tssrc/shared/settings/keys.tssrc/shared/settings/registry.tstest/integration/server/payments/sales-off-safety.test.tstest/integration/server/webhooks/refund-skip-conditions.test.tstest/shared/db/settings/public-api.test.tstest/shared/payments.test.tstest/shared/settings/keys.test.tstest/shared/settings/registry.test.ts
The shared type refactor shifted the payments.ts ?? equivalent from line 379 to 387; record the same equivalent in settings.ts:471 (data.payment_provider ?? "" — a provider type or null, never falsy-but-non-null).
CodeRabbit: tighten 'never stranded' to 'payments captured by that provider stay refundable and completable' in the acceptance doc and TODO, since the fallback only resolves the remembered provider (not per-charge). CodeRabbit: replace manual try/finally stub disposal with using declarations in sales-off-safety.test.ts (the established pattern in test/integration/server/webhooks/).
stefan-burke
commented
Aug 1, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fbb3d6d9a
ℹ️ 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".
Codex: throw on a corrupt non-empty last_active_payment_provider value (offensive programming — a stored garbage string is surfaced at read time rather than silently treated as null). Covered by a regression test. Codex: paymentDashboardUrl and the domain-change webhook warning both read settings.paymentProvider (the new-sales gate) and returned null when sales were off, hiding dashboard links and the domain-change warning for existing payments. Both now fall back to settings.lastActivePaymentProvider. Remove the broken CODEX_SECURITY_PYTHON export from flake.nix — the codexSecurityPython variable was never defined (landed on main in commit 09e47fd), breaking nix develop and every nix-wrapped check runner.
Pre-existing none sites: the resolver now falls back to the sole provider with stored credentials when exactly one is configured (unambiguous evidence). When zero or multiple providers have credentials, it returns null (genuine ambiguity — operator must re-select, no guessing). Concurrency race: setPaymentProviderNone now reads the current provider from the database via a SQL subquery inside the batch statement (not from the request-start snapshot), so a concurrent activation landing before the batch is correctly reflected. Uses a single INSERT OR REPLACE ... SELECT that evaluates subqueries against pre-statement state, making read and write atomic without withTransaction's cache invalidation (which resets the session private key). Regression tests: single-credential recovery, multi-credential no-guess, sumup sole-credential, concurrency race (stale snapshot vs DB state). Raw-cache mirroring assertions kill the syncStoredSetting removal mutant. Mirror-located tests added for require-provider, refunds, webhooks, settings-page, custom-domain, subdomain.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/shared/db/settings/raw-writes.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
syncWrittenBatchand reuse it insetPaymentProviderNoneto fix the doc comment and remove duplicated cache-sync logic. The comment onsyncWrittenBatchclaims it is "Shared by every batch write path," butsetPaymentProviderNonebypasses it, manually callingsyncStoredSettingtwice with the same key/value-mirroring logic thatsyncWrittenBatchalready implements in onesyncCachecall.
src/shared/db/settings/raw-writes.ts#L95-105: exportsyncWrittenBatch, or correct the comment to state it is shared only bywriteRawBatch-based paths.src/shared/db/settings.ts#L507-512: once known values are available (after the write completes), callsyncWrittenBatch([[CONFIG_KEYS.PAYMENT_PROVIDER, "none"], [CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, lastActive]])instead of two separatesyncStoredSettingcalls.🤖 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/settings/raw-writes.ts` at line 1, The syncWrittenBatch helper’s documented shared usage is incomplete and setPaymentProviderNone duplicates its cache synchronization. Export syncWrittenBatch from raw-writes.ts, then update setPaymentProviderNone to call it after the write completes with both payment-provider key/value pairs, replacing the two syncStoredSetting calls.test/shared/db/settings/public-api.test.ts (1)
133-141: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReload after the second
nonesave.This test only checks the current snapshot. A second
setPaymentProviderNone()call could clear the stored remembered provider while this assertion still passes.Invalidate the cache, reload
CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, and assert that the value remains"stripe".Proposed test extension
await settings.update.setPaymentProviderNone(); await settings.update.setPaymentProviderNone(); expect(settings.paymentProvider).toBeNull(); expect(settings.lastActivePaymentProvider).toBe("stripe"); + + settings.invalidateCache(); + await settings.loadKeys([CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER]); + expect(settings.lastActivePaymentProvider).toBe("stripe");As per coding guidelines, “Every bug fix must include a regression test that reproduces the exact defect and fails before the fix.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shared/db/settings/public-api.test.ts` around lines 133 - 141, The test “a second none save keeps the remembered provider” only verifies the in-memory snapshot; invalidate the settings cache after the second setPaymentProviderNone() call, reload CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, and assert the persisted value remains "stripe" alongside the existing assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation/equivalent-mutants/shared-m-z.txt`:
- Around line 29-33: Remove the three ?? → || entries for custom-domain.tsx and
subdomain.tsx from the equivalent-mutant list, since empty-string
paymentProvider values make them behaviorally different. Correct the
accompanying falsy-value statement so it does not claim that "none" is falsy; do
not narrow the state type or alter advancedDefaultState unless choosing that
alternative instead.
In `@src/shared/db/settings.ts`:
- Around line 473-516: Update setPaymentProviderNone to use
executeBatchWithResults, add RETURNING key, value to the INSERT statement, and
extract the returned last_active_payment_provider row instead of calling
requireOne. Protect the subsequent syncStoredSetting calls from a concurrent
activation so this operation cannot overwrite newer cached provider state with
the stale "none" state; use the existing cache synchronization mechanism.
In `@src/shared/payment-dashboard.ts`:
- Around line 33-36: Add a regression test for paymentDashboardUrl that sets
payment_provider to null and last_active_payment_provider to "stripe", then
asserts paymentDashboardUrl("pi_123") returns the expected Stripe dashboard URL.
Place it alongside the existing dashboard tests and preserve the current
direct-provider cases.
---
Outside diff comments:
In `@src/shared/db/settings/raw-writes.ts`:
- Line 1: The syncWrittenBatch helper’s documented shared usage is incomplete
and setPaymentProviderNone duplicates its cache synchronization. Export
syncWrittenBatch from raw-writes.ts, then update setPaymentProviderNone to call
it after the write completes with both payment-provider key/value pairs,
replacing the two syncStoredSetting calls.
In `@test/shared/db/settings/public-api.test.ts`:
- Around line 133-141: The test “a second none save keeps the remembered
provider” only verifies the in-memory snapshot; invalidate the settings cache
after the second setPaymentProviderNone() call, reload
CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, and assert the persisted value remains
"stripe" alongside the existing assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd6eada2-ebfb-4da1-a0da-abcec4751031
📒 Files selected for processing (26)
TODO.mddocs/payment-aggregate-acceptance.mdflake.nixscripts/mutation/equivalent-mutants/shared-db.txtscripts/mutation/equivalent-mutants/shared-m-z.txtsrc/features/admin/settings-page.tssrc/shared/db/settings.tssrc/shared/db/settings/raw-writes.tssrc/shared/payment-dashboard.tssrc/shared/payments.tssrc/ui/templates/admin/settings-advanced.tsxsrc/ui/templates/admin/settings.tsxsrc/ui/templates/admin/settings/custom-domain.tsxsrc/ui/templates/admin/settings/subdomain.tsxtest/features/admin/require-provider.test.tstest/features/admin/settings-page/last-active-provider.test.tstest/features/api/payment-processing/refunds.test.tstest/features/api/webhooks.test.tstest/integration/server/payments/sales-off-safety.test.tstest/integration/server/webhooks/refund-skip-conditions.test.tstest/shared/db/settings/public-api.test.tstest/shared/payments.test.tstest/ui/templates/admin/settings-advanced/state.tstest/ui/templates/admin/settings-state.tstest/ui/templates/admin/settings/custom-domain.test.tstest/ui/templates/admin/settings/subdomain.test.ts
💤 Files with no reviewable changes (1)
- flake.nix
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5345a26401
ℹ️ 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".
Finding (1): Replace the lagging-replica readOne after the INSERT...SELECT with RETURNING value — the computed last_active is read from the write's own batch result, never from a replica. Finding (2): Unify paymentDashboardUrl and settings-page.ts on the shared existingPaymentProviderType() from payments.ts — one provider-resolution mechanism, no duplicate credential scans. Templates restored to main (untouched); the advanced settings-page state supplies the resolved provider via s.paymentProvider, so DomainPaymentWebhookWarning sees it without template changes. Finding (3): Remove the invalid shared-db equivalent entry for none->empty. Finding (4): Remove the invalid UI ??->|| equivalent entries (string|null). Finding (6): Document INSERT...SELECT (not withTransaction) in TODO.md. Template files (.tsx) reverted to main to reduce mutation scope. Two settings-state test fixtures reverted to match.
…tActive
Replace rawWritesApi + manual branch checks + results[0]! with a single
valibot tupleWithRest schema that validates the full batch result shape:
exactly one result set with exactly one {value: string} row, plus any
remaining result sets. v.parse(results)[0].rows[0].value is fully
type-safe — no local invalid-shape branches, no non-null assertions, no
test-only exports.
Restore distinct lastActivePaymentProvider in template state (not
overloaded into paymentProvider which controls the sales radio).
Templates use s.paymentProvider ?? s.lastActivePaymentProvider so the
domain-change warning shows when sales are off while the radio stays
'none'. Template test fixtures and types updated to match.
Remove all jscpd:ignore markers outside import blocks; extract shared
test helpers (log-spy, webhook-verify-helpers, withRefundRedirect).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17a9483505
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/settings.ts (1)
250-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep provider selection in one atomic path.
lastActivePaymentProvidercorrectly maps""tonulland throws for unsupported values. Square and SumUp use the shared setter, but Stripeactivate()writesPAYMENT_PROVIDERbefore the shared setter writesLAST_ACTIVE_PAYMENT_PROVIDER. Include both keys in one atomic activation path.🤖 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/settings.ts` around lines 250 - 260, The Stripe activation flow must update PAYMENT_PROVIDER and LAST_ACTIVE_PAYMENT_PROVIDER atomically. Modify Stripe’s activate() path to use the shared setter or atomic mechanism that writes both settings together, removing the separate PAYMENT_PROVIDER write while preserving lastActivePaymentProvider’s empty-value and validation 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 `@test/features/api/webhooks.test.ts`:
- Around line 33-36: Update the “rejects a success callback with only bad token
params” test to store the response text and assert it contains the specific
invalid-payment-callback message used by the sibling test, replacing the
non-empty-length assertion.
In `@test/shared/db/settings/raw-writes.test.ts`:
- Around line 66-79: Add a negative test alongside the existing
“executeSettingsBatchReturningValue” test that exercises a RETURNING result with
zero or multiple rows and asserts the validation rejects it. Cover the newly
added batchReturningValue parse-rejection branch directly while preserving the
existing successful single-row test.
- Around line 33-37: Update the “throws on an empty batch” test callback to be
async and await the rejects assertion for writeRawBatch([]), ensuring the test
waits for and correctly reports the expected rejection.
In `@test/test-utils/log-spy.ts`:
- Around line 17-18: Remove the useDebugLogSpy re-export from this helper module
and update all callers to import it directly from its defining ./debug-log.ts
module; do not move or duplicate the implementation, and do not retain a
compatibility import path.
- Around line 20-22: Extract the duplicated call-matching logic from errorLogged
and debugLogged into one private logLogged helper that accepts the spy and
needle, preserves the explicit boolean return type, and has both exported
functions delegate to it without changing behavior.
In `@test/test-utils/webhook-verify-helpers.ts`:
- Around line 9-23: The webhook test helper does not expose the HTTP status, so
the deleted-listing refund test cannot verify its promised 404 response. In
test/test-utils/webhook-verify-helpers.ts lines 9-23, update withWebhookVerify’s
assertions callback to receive res.status and pass it through; in
test/features/api/payment-processing/refunds.test.ts lines 164-182, update the
deleted-listing refund test callback to assert that status is 404 while
preserving the existing JSON assertion.
In `@test/ui/templates/admin/settings/custom-domain.test.ts`:
- Around line 19-20: Strengthen warning assertions in
test/ui/templates/admin/settings/custom-domain.test.ts:19-20 and
test/ui/templates/admin/settings/subdomain.test.ts:19-20 by asserting the
complete provider warning structure, including its linked provider settings
target. At test/ui/templates/admin/settings/custom-domain.test.ts:32 and
test/ui/templates/admin/settings/subdomain.test.ts:32, assert that both the
complete warning element and provider link are absent, using structural or exact
observable assertions rather than independent presence-only checks.
---
Outside diff comments:
In `@src/shared/db/settings.ts`:
- Around line 250-260: The Stripe activation flow must update PAYMENT_PROVIDER
and LAST_ACTIVE_PAYMENT_PROVIDER atomically. Modify Stripe’s activate() path to
use the shared setter or atomic mechanism that writes both settings together,
removing the separate PAYMENT_PROVIDER write while preserving
lastActivePaymentProvider’s empty-value and validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5616ebe7-d126-4c16-86d4-71ab49e89259
📒 Files selected for processing (16)
TODO.mdscripts/mutation/equivalent-mutants/shared-m-z.txtsrc/features/admin/settings-page.tssrc/shared/db/client.tssrc/shared/db/settings.tssrc/shared/db/settings/raw-writes.tssrc/shared/payment-dashboard.tssrc/shared/payments.tssrc/ui/templates/admin/settings.tsxtest/features/api/payment-processing/refunds.test.tstest/features/api/webhooks.test.tstest/shared/db/settings/raw-writes.test.tstest/test-utils/log-spy.tstest/test-utils/webhook-verify-helpers.tstest/ui/templates/admin/settings/custom-domain.test.tstest/ui/templates/admin/settings/subdomain.test.ts
Add comprehensive render-snapshot tests for custom-domain.tsx and subdomain.tsx that assert every form field, action, label, CSS class, and i18n-rendered text — killing ~46 string-literal and operator mutants. Add a check-form test for subdomain.tsx covering the no-subdomain render path (type=text, muted, Check button, suffix). Remove stale equivalent-mutant entries: settings-page.ts:128:46 (line no longer has ??) and settings-bundles.ts:247:40 (superseded by 248:40 in shared-m-z.txt).
Contributor
There was a problem hiding this comment.
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/ui/templates/admin/settings/custom-domain.test.ts`:
- Around line 47-63: Strengthen the assertions at
test/ui/templates/admin/settings/custom-domain.test.ts:47-63 by matching the
custom_domain input value tickets.example.com and verifying it is associated
with the custom-domain form. In
test/ui/templates/admin/settings/subdomain.test.ts:47-63, assert the hidden
subdomain control contains my-sub and belongs to the registration form. In
test/ui/templates/admin/settings/subdomain.test.ts:76-79, bind the
availability-check input and its control to the check form, replacing
independent presence-only assertions with form-scoped observable checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: df80243a-a347-49a9-8b09-5debd683ebcf
📒 Files selected for processing (4)
scripts/mutation/equivalent-mutants/shared-a-l.txtscripts/mutation/equivalent-mutants/shared-m-z.txttest/ui/templates/admin/settings/custom-domain.test.tstest/ui/templates/admin/settings/subdomain.test.ts
💤 Files with no reviewable changes (2)
- scripts/mutation/equivalent-mutants/shared-m-z.txt
- scripts/mutation/equivalent-mutants/shared-a-l.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c67112c19
ℹ️ 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".
refunds.ts (16/16 killed): direct unit tests for tryRefund, validationFailure, refuseMismatch, refundSpec, refundedNoteText — table-driven for REFUND_REASONS. webhooks.ts (22/47 killed): Square orderId redirect, 409 concurrent reservation, refunded-with-failed-refund, augmented log assertions. custom-domain.tsx (8/8 killed): CSS-class, whitespace, and ??→|| assertions via regex-based render checks. subdomain.tsx (15/15 killed): i18n keys, CSS classes, ??→||, &&→||, !→∅, false→true, value→ via register+check form assertions. raw-writes.ts (4/6 killed): recordSettingsLoaded and loaded.add removal via audit-wrapped assertions. Remaining 29 survivors: webhooks.ts complex processSessionAndRedirect paths (multi-token URLs, thank-you), raw-writes loaded.add (not read by getCachedRaw), client.ts false→true (transient-error retry).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/features/api/payment-processing/refunds.test.ts`:
- Around line 314-322: Strengthen the non-404 case in the validationFailure test
by asserting an observable refund attempt outcome, not only refunded: false.
Reuse the existing errorLogged/errorSpy pattern for the unconfigured provider,
or configure a stub via stubStripeRefund and assert its specific result, so the
test distinguishes an attempted refund from skipping the non-404 branch.
In `@test/shared/db/settings/raw-writes.test.ts`:
- Around line 56-67: The test currently verifies only cache state and does not
confirm persisted deletion. Split the deletion behavior from the
audit-registration case, then in the focused `writeOrDelete` test invalidate the
cache, reload `CONFIG_KEYS.SQUARE_LOCATION_ID`, and assert
`settings.square.locationId` is empty while retaining the existing write/delete
setup.
In `@test/ui/templates/admin/settings/custom-domain.test.ts`:
- Around line 33-35: Remove the validation-action assertion from the
warning-suppression test and place it in the existing form-rendering test, or
create a dedicated test for it. Keep the current test focused exclusively on
verifying warning suppression, while preserving the validation action
expectation separately.
- Line 36: Replace the syntax-focused comments at
test/ui/templates/admin/settings/custom-domain.test.ts lines 36-36 and
test/ui/templates/admin/settings/subdomain.test.ts lines 33-34 with brief
behavior-focused statements that an empty configured provider suppresses the
warning; replace the comment at
test/ui/templates/admin/settings/subdomain.test.ts lines 44-45 with a brief
statement that the preview rendering path suppresses the warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 092d3bcf-ecdc-4709-9bf3-3fa7955060a5
📒 Files selected for processing (5)
test/features/api/payment-processing/refunds.test.tstest/features/api/webhooks.test.tstest/shared/db/settings/raw-writes.test.tstest/ui/templates/admin/settings/custom-domain.test.tstest/ui/templates/admin/settings/subdomain.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6dbcd7b09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aab800bcda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdefe2d718
ℹ️ 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".
…safety-behavior # Conflicts: # flake.nix # test/shared/stripe/endpoints/cleanup.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff70865d8d
ℹ️ 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".
stefan-burke
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 909b396027
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2676cc71b
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/settings-helpers.ts`:
- Around line 143-150: The repeated settings-task execution and error mapping
should be centralized in a shared helper. In
src/features/admin/settings-helpers.ts lines 143-150, define the helper to
accept taskName, formId, errorPage, expected version, and the operation, then
return either the operation result or the form error page; update
src/features/admin/settings-helpers.ts lines 408-423 and
src/features/admin/settings-general.ts lines 104-117 to call it instead of
duplicating withCurrentTask handling. In src/features/admin/settings-domains.ts
lines 45-58, build runGuardedTask on the shared helper and retain only its
recovery-guard behavior.
In `@src/locales/en/settings.json`:
- Line 28: Update the rejection path in the database settings logic near the
payment-provider activation validation to use the existing
error.payment_provider_activation_requires_recovery catalog key through t("key")
instead of the duplicated hardcoded sentence, keeping the English catalog entry
as the single source of user-facing copy.
In `@src/shared/db/settings.ts`:
- Around line 216-218: Update the call to syncLastActivePaymentProvider in the
settings flow to pass values[CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER] directly,
removing the ?? "" fallback. Preserve the existing behavior that allows a
missing required row or value to throw instead of silently treating it as no
remembered provider.
- Around line 183-201: Update the SQL construction and argument assembly near
the settings update flow to use explicit named or indexed bindings for all 24
placeholders instead of relying on the flattened positional arrays in args.
Ensure every repeated value and credential key maps to the correct placeholder,
and alias repeated settings subqueries with descriptive AS names where
applicable. Preserve the existing query behavior and conflict-update semantics.
In `@src/shared/db/settings/current-task.ts`:
- Around line 51-57: Remove the unreachable Number.isInteger guard after
executeWithoutCacheInvalidation in the current settings-version flow, and read
the guaranteed first row value directly via rows[0].value when computing
currentVersion. Preserve the existing query and version handling without adding
defensive fallbacks.
In `@test/features/admin/settings-helpers/provider-credentials.test.ts`:
- Around line 128-143: The test hardcodes the string "Another task is already in
progress" in the expectFlash call in the "does not save credentials while
another settings task runs" test function when it should import this message
constant from the source. Export the error message constant (and the STALE_TASK
constant) from src/shared/db/settings/current-task.ts, then import and use these
exported constants in the expectFlash assertion here and also fix the duplicate
hardcoded literal in test/features/admin/settings-domains/custom-save.test.ts
line 110 to use the same imported constant, ensuring test failures when source
messages change.
- Around line 43-59: Add a test for the credential-save route using the post
helper with a stale settings_version, and assert routes.save returns the
stale-version message without calling saveSecret or saveFields. Keep the
existing current-version behavior and target /admin/settings/test-provider
rather than the payment-provider route.
In `@test/test-utils/csrf.ts`:
- Around line 27-36: Update inputTagWithValue to throw when no matching input is
found instead of returning an empty string, and include both value and name in
the error message. Preserve the existing matching behavior for successful
lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d9b3446-afa0-47cb-87ba-dcfaa5554690
📒 Files selected for processing (25)
TODO.mdsrc/features/admin/settings-domains.tssrc/features/admin/settings-general.tssrc/features/admin/settings-helpers.tssrc/features/admin/settings-stripe.tssrc/features/api/payment-processing/refunds.tssrc/locales/en/settings.jsonsrc/shared/db/settings.tssrc/shared/db/settings/current-task.tssrc/shared/db/settings/raw-writes.tssrc/shared/existing-payment-provider.tssrc/shared/forms/csrf-form.tsxtest/features/admin/settings-domains/custom-save.test.tstest/features/admin/settings-general/payment-provider.test.tstest/features/admin/settings-helpers/provider-credentials.test.tstest/features/admin/settings-page/last-active-provider.test.tstest/features/admin/settings-stripe.test.tstest/integration/server/settings/sensitive-field-masking.test.tstest/shared/db/client/upstream-errors.test.tstest/shared/db/settings/public-api.test.tstest/shared/stripe/endpoints/connection.test.tstest/specs/steps/payment-provider-choice.tstest/test-utils/csrf-and-forms.test.tstest/test-utils/csrf.tstest/test-utils/session.ts
💤 Files with no reviewable changes (2)
- test/shared/db/client/upstream-errors.test.ts
- src/shared/db/settings/raw-writes.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 550b6ce92f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fba391d7e
ℹ️ 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
403
to
404
| path: string, | ||
| data: Record<string, string> = {}, |
There was a problem hiding this comment.
Adding the settings-version setup grows test/test-utils/session.ts from 519 to 529 lines, extending an already oversized test helper instead of moving adminFormPost and its related form-session helpers into a focused module. The repository applies its approximately 400-line limit to test files specifically to keep test and mutation scopes manageable.
AGENTS.md reference: AGENTS.md:L82-L82
Useful? React with 👍 / 👎.
…t we cannot read (#2021) * Reject malformed money and blank provider resource ids at the payment boundary The provider boundary now refuses a charge whose amount is not a non-negative minor-unit integer or whose currency is not three letters, so every webhook and success callback reads a well-formed charge. A blank or whitespace-only provider resource id is refused at tryRefund, the one path every refund shares; a captured charge is still kept and surfaced, never dropped. Refund state is typed (none/completed/unknown), with "unknown" valid only for a legacy charge whose refund was never observed. * Reject blank resource IDs and non-matching currencies at the boundary Codex review on PR #2021 flagged two gaps in the provider boundary: 1. A paid session with a blank payment reference was accepted and persisted, but the reference was then excluded by getRefundPaymentReferences and refused by tryRefund, stranding the captured money. The boundary now rejects a paid session whose resource id is blank, rather than persisting an unrefundable charge. 2. A missing currency was defaulted to the site's currency (??), and a different valid currency was silently accepted. Both are now refused: a missing expected field is a hard no, and a non-matching currency would put the amount in the wrong unit for the site-currency proof. * Address CodeRabbit findings on the provider boundary - Fix the stale comment on Stripe session currency: the boundary rejects a missing currency, it does not settle it to the site's. - Assert, rather than default to empty, the SumUp checkout currency; a missing expected field is a hard no. - Validate the SumUp checkout reference before loading staged metadata, and handle a missing staged row instead of asserting non-null. - Deduplicate the refund-state documentation (one concise statement of when unknown is valid). - Annotate exported payment-session test helpers with explicit return types. * Purge the codexSecurity python env var from the dev shell and fix the webhook fixture default - Remove the undefined codexSecurityPython reference from flake.nix (upstream main added the export without defining the binding, which broke nix develop) and delete the env var entirely; the dev shell no longer exports a codexSecurity python path. - checkoutSessionEvent now defaults payment_intent to a non-blank pi_<sessionId> so an omitted value still yields a processable paid session; pass null explicitly to exercise the boundary blank-reference rejection. Direct fixture tests live in dispatch.test.ts (a new file would reshuffle the suite's test groups and surface pre-existing isolation flakiness in guide schema and mutation-isolation coverage). * Keep payment schemas private, prove the refund guard, and extract the session boundary Codex re-review (P1) on PR #2021: - CurrencySchema/MoneySchema and ResourceIdSchema were exported only for their own tests; they are now module-private and the tests exercise the money() and isResourceId() production interfaces. - The resource-id module doc no longer compares the current guard with the old per-provider parsing. - The tryRefund blank-reference test now configures a provider and spies on refundPayment/retrievePaymentIntent, so it proves the guard fires before any provider call rather than merely returning false because no provider was resolvable. - validatedPaymentSession moves from the 800-line payment-helpers.ts into src/shared/payment/validated-session.ts; its test moves to the mirror location. * Validate the raw SumUp amount before rounding to minor units toSumupCheckout previously passed the provider's major-unit amount through toMinorUnits (which rounds) before the payment boundary validated it, so an over-precise charge could round to a value that matches the signed total. The raw amount is now checked against the currency's precision first and fails loudly. Regression test: a GBP checkout with a three-decimal amount resolves to no session. * Bind the SumUp staged record to the webhook id and address CodeRabbit re-review - resolveWebhookSession now requires the staged record's sumupId to equal the webhook id, so a provider response naming a different staged checkout cannot process the wrong booking; regression test stages two checkouts and a cross-reference. - RefundState is defined from a valibot picklist schema, matching the repository's schema-first value-type pattern. - The balanceSession fixture drops its unchecked double cast (created/url filled in, meta narrowed to string values). - The blank-reference webhook test asserts no processed-payment row is persisted. - The createdAt assertion now checks the constructed session directly; the fixture-only checkoutSessionEvent tests are removed (the default is exercised through the production webhook path). * Route wrong-currency paid charges into the existing mismatch refund path Codex P1: a paid charge in a different valid currency with a usable reference was rejected at the boundary, so the webhook acked it as unrecognized and the captured money was neither booked nor refunded. The session now carries its validated currency, and classifySession treats a non-site currency as a price mismatch, so the charge flows through the existing mismatch -> refund -> alert machinery instead of being dropped. * Refund paid charges the boundary cannot read, via a typed rejection A paid charge in an unusable shape (wrong currency, over-precise SumUp amount, malformed amount/currency) previously made resolveWebhookSession return null, so the webhook acked it as unrecognized and the captured money was stranded. validatedPaymentSession now returns a typed rejection that carries the payment reference and whether it is refundable; the providers surface it, and the webhook, redirect, and cancel paths refund a refundable rejected charge through tryRefund before acknowledging. SumUp flags over-precise amounts instead of throwing, so those reach the refund path too. Also: RefundStateSchema is private, the resource-id test comment no longer compares with old parsing, and the sumup-provider ?? -> || equivalents were removed (falsy non-string ids differ) and are now pinned by a test. * Convert SumUp charges in the checkout's own currency toSumupCheckout converted amounts and checked precision against the site currency, so a valid foreign-currency checkout (e.g. JPY, no minor places) was rounded or flagged against the wrong decimal places. The checkout currency is now normalized once and passed to toMinorUnits and exceedsCurrencyPrecision; both helpers default to the site currency for existing callers. Regression test: a one-decimal JPY amount converts and flags against JPY, not GBP. * Retry failed rejection refunds, verify ownership, and validate SumUp currency presence CodeRabbit + Codex re-review (9 findings): - refundRejectedCharge now returns the refund outcome, and a rejected-charge refund the provider refuses is answered 503 (retryable) by the webhook and a retryable error by the redirect/cancel paths instead of a 200 ack that strands the charge. - The malformed_charge rejection carries the session metadata, and the refund is issued only when its price proof verifies, so a session belonging to another instance sharing the provider account is not refunded. - isSessionRejection validates the exact variants and fields. - SumUp: a missing or blank checkout currency is carried as null (never asserted away inside withClient) so the boundary refuses it as a refundable malformed charge; currency conversion and precision checks take the checkout currency. - SessionBuild is module-private; stale providerRefunded comment updated; asSession(result!) simplified; the accidental npm:biome lock resolution (566 lines) removed and the bare specifier removed from the deno-command test. * Keep a malformed SumUp currency out of the currency helpers SumUp answering with a currency that is not a real three-letter code (say "GB") made the checkout fetch throw: the code went straight to Intl.NumberFormat for the minor-unit conversion and the precision check, and Intl rejects it. The client wrapper caught that throw and turned it into "no session", so the webhook acknowledged a paid charge as unrecognized and the captured money was neither booked nor refunded. The adapter now converts with the site's currency whenever the provider code is not well-formed, and carries the raw code through so the payment boundary refuses the charge and the callback refunds it — the same route a missing currency already took. The guard is the boundary's own currency schema, exposed as isCurrency, so a well-formed code has one definition. Also strengthens two assertions review asked for: the webhook 503 test proves the refund was attempted on the captured charge, and the currency-fallback tests assert the whole normalized checkout. Splits the two SumUp suites, which were over the 400-line guide, into mirror directories, with their fake client, fixtures, and suite setup in one #test-utils module. * Cover the currency guard directly isCurrency is a new exported guard; give it its own table of well-formed and malformed codes rather than leaning on the SumUp adapter's tests. * Carry a currency on the merged webhook fixture The base branch added a webhookEvent test fixture after this branch had updated the Stripe and Square fixtures to carry a currency, so every session it built was refused at the payment boundary once the two met. Give it the site currency, lower-cased the way Stripe sends it. The base's redirect-path refund suite and this branch's rejected-charge unit suite both landed at test/features/api/payment-processing/refunds .test.ts; they test different things, so the unit one moves to refunds/rejected-charge.test.ts and both stay under the line guide. * Close mutation gaps on the payment sources this layer touches Adds a direct table for parsePriceProof (moved into payment-signature.ts by this layer but only covered indirectly), a '-0' row proving the non-negative money parser refuses a signed zero, the sub-minor-unit sign cases for formatSignedCurrency, and the SumUp provider's webhook contract assertion its Stripe and Square siblings already had. Records two proven-equivalent mutants: the canonical key sort's -1/1 return (sort() only tests for a strictly negative result — checked against all 8! permutations and 3,000 random arrays) and the positive money schema's signed flag (a leading minus only admits values at or below zero, which minValue(1) rejects either way). Refreshes the webhook and SumUp ignore-list line numbers the merge shifted. * Refresh provider ignore-list line numbers the merge shifted * Bring every source this layer touches to a 100% mutation kill Stripe's provider adapter now hands the boundary the currency exactly as Stripe sent it, rather than turning an absent one into null on the way in; the boundary already refuses absent, blank, and malformed alike, so the extra defaulting only hid which of them arrived. New tests for the gaps the runs found: a paid Stripe session with no payment intent is refused as a blank reference, a one-character session id is still looked up, both webhook-endpoint states parse and an unknown one does not, a normal booking does not pick up the quantity-0 placeholder's note, and an unrelated system note survives the stale-note cleanup. Splits the Stripe provider's webhook tests into a mirror directory, and folds the two 503 webhook posts in the refund-skip suite onto one helper. * Stop three more captured charges dying inside the client wrapper Same fault as the malformed SumUp currency, in three more places. Each reached into provider data that may not be there, from inside the wrapper that turns any error into "no session" — so the webhook acknowledged a paid charge as one it had never heard of and the money stayed taken. - A SumUp checkout with no amount: the precision check called toFixed on undefined. The amount is carried as null for the boundary to refuse. - A Square order with no money object: retrieveOrder asserted its way into it. Both halves are carried as null instead, and the provider keeps a missing amount missing rather than letting Number(null) read as a real free order. - A Square rejection carried the packed wire record. The price proof is signed over the unpacked shape, so the rejection's own ownership check could never pass and a real Square charge would never be refunded. The rejection now carries the same canonical metadata the success path does. Also stubs the Stripe refund in the rejected-charge tests instead of spying it, so those refunds are answered here rather than leaving for Stripe, and folds their shared scaffolding onto one curried helper. Records the remaining review point — writing a durable "refused and refunded" outcome for the session — in TODO.md with why it is not a live fault today. * Close the mutation gaps the Square money change opened up square.ts is in this branch's diff now, so its survivors are ours. Four were in the refund response's boundary schema, which no test could distinguish because every malformed fixture was missing several fields at once: a blank refund id, a blank payment id, and a blank currency are now each refused on their own, and a zero refund amount is shown to parse and be caught by the amount check instead. A fifth was the log written when Square refuses a refund — the only record of it, since the caller just sees false. The two new nulls needed their own cover: a zero total and a blank currency are Square's real values and are carried through untouched, so only a wholly absent money object reads as null. * Say plainly when a buyer's money went back Two things were reading the same for a charge we refunded and a charge there was never anything to refund. The operator log said "refunded: true" either way, including for a blank-reference rejection where no provider call was ever made. The outcome now carries both facts apart: settled (nothing left owing, which decides the retry) and refunded (money actually moved, which is what gets logged). The buyer got "Payment session not found" even when they had really been charged and we had just sent the money back — an invitation to wait for a ticket that will never come, or to pay a second time. They are now told what happened, and that it takes a few days. Both callbacks answer a rejected session the same way, so they share one helper rather than two copies of refund-log-respond. Separately, a SumUp checkout that comes back under a reference we never staged is now raised as an error instead of returning quietly. The booking is encrypted under that reference, so an unmatched one leaves us unable to read it or prove the charge is ours to refund — which is exactly when someone needs to know. * Configure Stripe's key, not just its name, in the refund tests The base branch made an existing payment resolve only through a provider this site still holds credentials for, so naming stripe as the current provider no longer reaches it — every refund in this suite silently found none and returned false. The helper now sets the key alongside the choice. * Refuse a provider resource id with space around it " pi_123 " contains text, so the guard accepted it — and the id is stored and sent back to the provider exactly as it arrived, so the session was booked as refundable and then matched no charge on every refund attempt. The id must have no space around it; padding is refused rather than trimmed, since trimming guesses at what the provider meant. Records the other review point — telling a buyer whose money was taken and not given back, which the refunded page does not yet cover — in TODO.md, with the three outcomes it applies to and the one it must not. * Use the payment id Square signed, not just the order's tenders A payment.updated webhook carries a payment id Square has verified and reported COMPLETED, and the session build threw it away — it read the reference off order.tenders instead. Square's tenders can lag the payment, so a completed payment on an order whose tenders had not caught up read as unpaid: acknowledged as pending with the money taken, or, once its money was also unreadable, refused as a charge with nothing to refund. retrieveSession now takes that already-known payment as an optional second argument, on the shared provider interface rather than Square's copy of it: any provider whose session lags its payment has the same problem, and the ones that do not simply ignore it. * Let the webhook's payment win over an earlier tender Preferring the order's first tender only helped when there was no tender at all. An order can already carry a tender for an earlier payment, and the webhook names the one Square has just reported COMPLETED — so that is the charge this session records, and the one a refund has to reach. Also asserts the payment lookup happened before reading its arguments, so a regression reports the missing call rather than a TypeError, and notes on the deferred buyer-message work that a 503 only asks for another *webhook* delivery: the redirect and cancel paths have no retry behind them, so an unresolved refund has to be written down before anyone is told it is in hand. * Work through the review comments I had closed unread Ten threads had been resolved by a sweep without ever being read. Going back through them: - refundRejectedSession and RejectionOutcome had no production caller outside their own module, so they were public only for tests. Both are private now, and those tests go through answerRejectedSession — which asserts the buyer's page and status rather than an internal shape. - The "not found" wording is a catalog entry rather than a literal repeated across three call sites. - The refund copy is active voice: "we did not make a booking", "we have sent your money back". - The SumUp normalization comment was thirteen lines narrating each branch; it is four, keeping only the withClient constraint that is not visible from the code. - The refund-schema tests assert which field Valibot refused, not just that something threw — an unrelated fault would have passed before. - Non-null assertions I added are early-return narrows, a stale comment about "no session" describes the blank_reference rejection it actually asserts, two call-argument reads assert the call happened first, and the classify test pins the reference the refund was issued for. - square-provider.test.ts was 718 lines; its webhook cases move to the mirror directory, with the fixtures and suite hooks both files were about to duplicate lifted into the shared Square test utils. * Refuse whitespace anywhere in a provider resource id "pi 123" was accepted, and a test of mine asserted it should be. It never can be: the id goes back to the provider exactly as it arrived, so an id with a space in it books the session as refundable and then fails every refund — leaving the webhook retrying for good, which is worse than refusing it at the boundary. The whole id must be unbroken text now. Also from review: the payment-session boundary's eighteen-line comment narrated the branches below it and is four lines saying what the code cannot; "payment session not found" is a full sentence in the buyer's voice; the two callback tests hand their result back from the helper instead of a definite-assignment binding; and the Square webhook test asserts the whole order-lookup call list rather than its first entry. * Validate what Square took, and retry when it cannot be read back Two faults in the same lookup, both leaving a captured charge stranded. The webhook already saw Square report a payment COMPLETED, but completion was then re-derived solely from a second retrievePayment call. When that call failed transiently the status fell back to "unpaid" and the webhook acknowledged the charge as pending with a 200, so Square never delivered it again. A failed read-back of a payment the webhook verified now throws, which answers the caller retryably instead of going quiet. The money came from order.totalMoney — what was asked for, not what was taken. A partial payment therefore matched the order's signed price and booked as paid in full. Square's own figure for the payment is used whenever it names one, so a short charge is refused by the boundary and refunded. Simplifies to stay inside the source budget, none of it cosmetic: the SessionBuild wrapper and sessionOrRejection did the job isSessionRejection already does, so validatedPaymentSession returns the session or the rejection directly and three provider adapters stop unwrapping it; refundRejectedSession was a one-line private wrapper with a single caller and is inlined; and four comment blocks that narrated the code below them are cut to what the code cannot say. * Assert the flat session shape in the dispatch test too A second validatedPaymentSession test lived in the payment-helpers suite and still expected the { ok, session } wrapper that this branch removed. CI caught it; my targeted run had not reached that file. * Close three boundary gaps the contract audit found Auditing all three adapters against one written contract — trusted signed facts versus observed provider facts, and what a completed result must carry — turned up two providers each missing a guard the other already had, plus one place expected money still stood in for observed money. Square never checked that the payment it fetched belongs to the order it is about to book. Square's two records disagreeing meant booking one order's signed metadata against another payment's money. It now skips, the way SumUp already does when a checkout comes back under a reference that is not the one staged for it. The payment's own order is left alone, so its real webhook still books it. SumUp acknowledged a staged checkout it could not fetch. The staging row had already proved the checkout was ours, so a failed read is SumUp being unreachable, not a checkout we never made — and a 200 is terminal, so a paid one was stranded. It now throws, the way Square already does for a payment it cannot read back. A completed Square payment that named no amount fell back to the order total, which is what was asked for rather than what was taken — the same hazard as a partial charge, reached a different way. Observed money is now required exactly when money was observed to move; before that the order total is all there is and nothing has been captured against it. The COMPLETED payment fixtures gain the amountMoney real Square always sends, and one test named for the order total is renamed for what it now proves. * Fold SumUp's over-precise flag into an unreadable amount overPrecise was an optional field that changed what amountMinor meant: when set, the number beside it was a lie. It also gave the SumUp adapter its own route to a rejection, in parallel with the boundary that decides this for every other provider. An amount finer than its currency can hold is simply an amount we cannot read, so it is now carried as null like an absent one, and the one boundary refuses both. Behaviour is identical — same reference, same paid flag, same refundable rejection — and the refusal is now logged, which the adapter's own path never did. Drops the field, its branch, and a table row that had become a duplicate of the no-amount case. * Retry a Square payment whose order contradicts the signed event I had this one wrong. The mismatch was treated as "this payment belongs to another order, whose own webhook will book it", so it skipped — and skip is a 200, which is terminal. But the signed event that sent us here had already linked the payment to this order, so a lookup naming a different one is Square disagreeing with itself about money it has taken, not a routine delivery for somebody else. There may be no second webhook coming. Neither answer available at that point is safe: booking uses metadata the payment may not belong to, and acknowledging strands a captured charge. Throwing keeps it retryable until Square answers consistently, which is what the other two contradictions in this file already do. * Retry a Square payment that does not read back as completed A signed webhook reports a payment COMPLETED, but the follow-up read can come back missing or still saying APPROVED. Either way the session was built as unpaid, and the webhook then acknowledged captured money as pending, so Square had no reason to send it again. Both readings are Square lagging its own signed event, so they now share one guard and throw, keeping the delivery retryable until Square agrees with itself. Also condenses the money builder's comment to the part the code does not already say, and records the foreign-currency money history finding in TODO.md: filing it honestly needs a currency on the accounting store itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Retry a SumUp checkout that comes back under the wrong reference The staging row already proved this checkout is ours, so a reference SumUp answers with that is blank, unknown, or another booking's is SumUp contradicting itself. The code logged that and returned null, which the webhook acknowledged as unrecognized, so a paid checkout was left with the money taken, no booking, and no refund. The comment beside it already said to raise it. It now throws, keeping the delivery retryable. The booking is encrypted under that reference, so without a match there is nothing we can read or prove is ours to refund — there is no safe way to answer this one quietly. Also drops the export from malformedChargeRejection, whose only caller is in its own module, refreshes the SumUp header now that a checkout converts in the currency SumUp returned, and condenses the resource-id comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Condense the rejected-session callback comment Keeps the retry constraint, which the code does not say for itself, and drops the narration of the refund, the log line, and the buyer's message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adab54eb2b
ℹ️ 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
+76
to
+77
| if (rejection.reason === "blank_reference" || !rejection.refundable) { | ||
| return NOTHING_TO_REFUND; |
There was a problem hiding this comment.
When SumUp reports a checkout as PAID before returning its optional transaction ID, validatedPaymentSession produces blank_reference, but this branch marks the charge settled. The webhook then acknowledges it at webhooks.ts:426-427, permanently leaving the buyer charged without a booking or refund. A blank reference on this rejection means the expected provider field is unavailable, not that no money was captured; keep it unsettled so the webhook returns 503 and retries until the reference can be read.
AGENTS.md reference: AGENTS.md:L118-L127
Useful? React with 👍 / 👎.
| }, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This newly added test file is 434 lines, already exceeding the repository's approximately 400-line limit for tests. Split the webhook session, payment readback, and event-shape cases into narrower suites so Square mutants do not need to run against another oversized test file.
AGENTS.md reference: AGENTS.md:L82-L82
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Turning off new sales now stops new checkouts without cutting off payment work that already exists.
Operator impact
An operator can pause new online sales and still refund or finish payments already in progress. If an older site has an unclear provider, the settings page asks for an explicit choice while keeping new sales off. A stale settings page asks the operator to reload instead of overwriting a newer choice.
Known limitation
Each payment still stores only its provider reference, not the provider name. If a site took payments through more than one provider, older payments may still need per-payment provider tracking. That follow-up is recorded in
TODO.mdanddocs/payment-aggregate-acceptance.md.The requested splits for the oversized settings and webhook modules are also recorded in
TODO.md. They were not included here because this PR has a strict source-change limit.Verification
main: 556 additions + 158 deletions = 714 lines.