Accept only confirmed Square refund outcomes - #1911
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSquare refund handling now parses refund responses and confirms success only for approved statuses with required fields. Tests cover response mapping, status outcomes, and malformed data; mutation records, assertions, fixtures, and follow-up TODOs are also updated. ChangesSquare refund confirmation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
e6d96be to
5a795fa
Compare
![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: e6d96be010
ℹ️ 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 review pointed out that returning false for a refund response missing its object/id/status silently swallowed a Square contract break as an ordinary not-yet-refunded payment — the codebase's offensive-programming rule says a missing documented field is a hard no that must fail at the boundary. refundPayment now throws when the 200 response lacks a refund object, id, or status; withClient contains the throw into a logged false, so the bad response is diagnosable instead of silently false. PENDING/FAILED/unknown statuses still return false silently (only a present-but-not-confirmed status is a normal not-yet-refunded outcome). Adds a test asserting the boundary error log; the existing fails-safely tests still hold (the throw is contained to the same false outcome). The pending-result union Codex also suggested (propagate a pending refund id instead of false) is the staged-checkout/callback work this PR was told not to introduce and is recorded in TODO.md against #1853/#1905. Refreshed the surviving equivalent-mutant line numbers in equivalent-mutants.txt (the helper removal shifted result ?? false and locations ?? []).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3d4ec216d
ℹ️ 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 follow-up (thread 3): the boundary throw lived inside withClient, which catches non-PaymentUserError exceptions and returns null, so it was architecturally the same as a logged false — an indirect throw-and-catch rather than a real loud failure. Replaced it with an explicit logError + return false, matching the missing-amount case just above it: the malformed response is still diagnosed at the boundary under E_SQUARE_REFUND and still fails safely to false (never reports a refund that was never confirmed), now without the swallowed-throw indirection. Propagating the throw outside withClient (a loud/errored failure) would change the contract from fail-safely-contained to fail-loudly, which is the per-path staged-checkout/callback resolution this PR was told not to introduce and is recorded in TODO.md against #1853 and #1905. Exhaustive mutation on square.ts is now 100% (286 killed, 3 genuine equivalents suppressed).
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 (1)
src/shared/square.ts (1)
638-675: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not treat an accepted
PENDINGrefund as a retryable failure.Line 675 returns
falseforPENDING; downstream callers interpret that as failure. The documented flow then releases the reservation, returns 503, and retries the full refund with a new idempotency key rather than resolving the existing refund ID. Preserve a distinct pending outcome or otherwise make redelivery query/reuse the accepted refund before this behavior ships.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/square.ts` around lines 638 - 675, Update refundPayment and its callers so an accepted Square PENDING refund is represented distinctly from FAILED or malformed refunds; ensure redelivery queries or reuses the existing refund ID rather than releasing the reservation and retrying with a new idempotency key. Preserve true only for CONFIRMED_REFUND_STATUSES and keep malformed responses fail-safe.
🤖 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.txt`:
- Line 897: Update the equivalence rationale for the src/shared/square.ts
callback mutation to mention that pending, unknown, and malformed refunds now
return false, and that false ?? false and false || false both evaluate to false.
Preserve the existing rationale for true results and null error or
missing-client cases.
In `@src/shared/square.ts`:
- Around line 235-242: Update the SquareRefundResponse definition and refund
POST handling to use a Valibot schema as the source of truth, with an optional
refund object whose id and status fields are optional strings. Parse and
validate the response before mapping or applying the unconfirmed-refund
fallback, rejecting malformed JSON and invalid field types rather than relying
on the post<SquareRefundResponse> cast.
In `@test/shared/square/retrieve-refund.test.ts`:
- Around line 421-422: Update the comment preceding the malformed-response
assertion in the refund test to state that the implementation logs the malformed
response and returns false. Remove the inaccurate claim that withClient throws
or that the error is silently handled, while preserving the existing assertion
and test behavior.
---
Outside diff comments:
In `@src/shared/square.ts`:
- Around line 638-675: Update refundPayment and its callers so an accepted
Square PENDING refund is represented distinctly from FAILED or malformed
refunds; ensure redelivery queries or reuses the existing refund ID rather than
releasing the reservation and retrying with a new idempotency key. Preserve true
only for CONFIRMED_REFUND_STATUSES and keep malformed responses fail-safe.
🪄 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: a17e7d78-23d3-49ef-b290-64e078b98bc2
📒 Files selected for processing (9)
TODO.mdscripts/mutation/equivalent-mutants.txtsrc/shared/square.tstest/shared/square-provider/provider.test.tstest/shared/square/client.test.tstest/shared/square/payment-link-validation.test.tstest/shared/square/rest-transport.test.tstest/shared/square/retrieve-refund.test.tstest/shared/square/webhook.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71c42ba0b5
ℹ️ 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".
…1911 review) CodeRabbit noted the recorded proof for the result ?? false equivalent mutant predated the new false return path. The callback now returns true (confirmed), false (pending/failed/unknown status, or a malformed response), or null (missing client or a caught error); true stays true and both false and null fall through to false under ?? and ||, so the operators still produce the same boolean. The rationale now states that.
CodeRabbit and Codex flagged that Square's RefundStatus enum includes APPROVED (the current terminal success state for a refund), which CONFIRMED_REFUND_STATUSES omitted — a refund Square approved would have been wrongly treated as not-confirmed. Added APPROVED alongside COMPLETED and SUCCEEDED (kept per the job spec as defensive aliases), with a regression test. Confirmed APPROVED against Square's current API reference. Also corrected the now-stale comment in the malformed-response test (it no longer throws; it logs under E_SQUARE_REFUND and returns false). Exhaustive mutation on square.ts is still 100% (288 killed, 3 genuine equivalents suppressed; line numbers refreshed).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/shared/square.ts (2)
643-649: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
APPROVEDas a confirmed refund status.The implementation returns
trueforAPPROVED, but this JSDoc lists onlyCOMPLETEDandSUCCEEDED. Update the comment so the documented contract matches the allowlist and tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/square.ts` around lines 643 - 649, Update the JSDoc for the refund method near the full-refund implementation to include APPROVED alongside COMPLETED and SUCCEEDED as confirmed statuses, keeping the existing descriptions of pending, failed, unknown, and malformed responses unchanged.Source: Coding guidelines
661-680: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not collapse an accepted
PENDINGrefund into plainfalsewithout updating downstream handling.Line 680 makes an unsettled refund indistinguishable from failure. The supplied TODO states that webhook/admin paths release the reservation, return 503, and retry with a fresh idempotency key instead of polling the existing refund id. Propagate a pending outcome carrying
refund.id, or update those callers in the same change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/square.ts` around lines 661 - 680, Update the refund handling around the refund-status check in the payment refund method so an accepted PENDING refund is propagated as a distinct pending outcome carrying refund.id, rather than returned as plain false. Update all downstream webhook/admin callers to recognize that outcome, retain the reservation, return 503, and retry using the existing refund id instead of treating it as a failed refund.
🤖 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.txt`:
- Around line 896-898: Resolve the target-branch content conflict in
scripts/mutation/equivalent-mutants.txt, preserving the correct
equivalent-mutant entries from both branches without duplicate or stale records.
Then rerun the merge check and mutation audit to confirm the file is clean and
consistent.
---
Outside diff comments:
In `@src/shared/square.ts`:
- Around line 643-649: Update the JSDoc for the refund method near the
full-refund implementation to include APPROVED alongside COMPLETED and SUCCEEDED
as confirmed statuses, keeping the existing descriptions of pending, failed,
unknown, and malformed responses unchanged.
- Around line 661-680: Update the refund handling around the refund-status check
in the payment refund method so an accepted PENDING refund is propagated as a
distinct pending outcome carrying refund.id, rather than returned as plain
false. Update all downstream webhook/admin callers to recognize that outcome,
retain the reservation, return 503, and retry using the existing refund id
instead of treating it as a failed refund.
🪄 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: 9ecc072a-c318-4241-a6a8-3f73ec99c0a9
📒 Files selected for processing (4)
TODO.mdscripts/mutation/equivalent-mutants.txtsrc/shared/square.tstest/shared/square/retrieve-refund.test.ts
Codex review pointed out that returning false for a refund response missing its object/id/status silently swallowed a Square contract break as an ordinary not-yet-refunded payment — the codebase's offensive-programming rule says a missing documented field is a hard no that must fail at the boundary. refundPayment now throws when the 200 response lacks a refund object, id, or status; withClient contains the throw into a logged false, so the bad response is diagnosable instead of silently false. PENDING/FAILED/unknown statuses still return false silently (only a present-but-not-confirmed status is a normal not-yet-refunded outcome). Adds a test asserting the boundary error log; the existing fails-safely tests still hold (the throw is contained to the same false outcome). The pending-result union Codex also suggested (propagate a pending refund id instead of false) is the staged-checkout/callback work this PR was told not to introduce and is recorded in TODO.md against #1853/#1905. Refreshed the surviving equivalent-mutant line numbers in equivalent-mutants.txt (the helper removal shifted result ?? false and locations ?? []).
Codex follow-up (thread 3): the boundary throw lived inside withClient, which catches non-PaymentUserError exceptions and returns null, so it was architecturally the same as a logged false — an indirect throw-and-catch rather than a real loud failure. Replaced it with an explicit logError + return false, matching the missing-amount case just above it: the malformed response is still diagnosed at the boundary under E_SQUARE_REFUND and still fails safely to false (never reports a refund that was never confirmed), now without the swallowed-throw indirection. Propagating the throw outside withClient (a loud/errored failure) would change the contract from fail-safely-contained to fail-loudly, which is the per-path staged-checkout/callback resolution this PR was told not to introduce and is recorded in TODO.md against #1853 and #1905. Exhaustive mutation on square.ts is now 100% (286 killed, 3 genuine equivalents suppressed).
CodeRabbit and Codex flagged that Square's RefundStatus enum includes APPROVED (the current terminal success state for a refund), which CONFIRMED_REFUND_STATUSES omitted — a refund Square approved would have been wrongly treated as not-confirmed. Added APPROVED alongside COMPLETED and SUCCEEDED (kept per the job spec as defensive aliases), with a regression test. Confirmed APPROVED against Square's current API reference. Also corrected the now-stale comment in the malformed-response test (it no longer throws; it logs under E_SQUARE_REFUND and returns false). Exhaustive mutation on square.ts is still 100% (288 killed, 3 genuine equivalents suppressed; line numbers refreshed).
99c3d5a to
dccfb50
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dccfb50d58
ℹ️ 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".
All concerns from this review are addressed — every thread is resolved, and the branch was rebased onto main (absorbing #1909/#1910/#1912, including #1912 stable Square refund idempotency keys). Dismissing because this changes-requested was against the pre-rebase commit 99c3d5a, which is no longer in the branch; CodeRabbit already re-reviewed the rebased tip as COMMENTED with no new findings.
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/shared/square.ts`:
- Around line 567-576: Limit CONFIRMED_REFUND_STATUSES in
src/shared/square.ts:567-576 to COMPLETED and SUCCEEDED by removing APPROVED.
Update test/shared/square/retrieve-refund.test.ts:410-414 to remove or change
the APPROVED success expectation so tests enforce only the declared confirmation
statuses.
🪄 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: 6dba3257-9e88-40ad-a69f-66f0720a1522
📒 Files selected for processing (9)
TODO.mdscripts/mutation/equivalent-mutants.txtsrc/shared/square.tstest/shared/square-provider/provider.test.tstest/shared/square/client.test.tstest/shared/square/payment-link-validation.test.tstest/shared/square/rest-transport.test.tstest/shared/square/retrieve-refund.test.tstest/shared/square/webhook.test.ts
…iew) Codex follow-ups: the refundPayment docstring still listed only COMPLETED/SUCCEEDED after APPROVED was added (now references CONFIRMED_REFUND_STATUSES), and the malformed-response guard only checked truthiness, so a truthy non-string id (e.g. id: 1) with an allow-listed status would have been recorded as a confirmed refund — the guard now requires string id/status (a missing or non-string field logs at the boundary under E_SQUARE_REFUND and returns false). Exhaustive mutation on square.ts is 100% (301 killed, 3 equivalents suppressed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f68e7b444
ℹ️ 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".
…#1911 review) Codex P2 with Square doc sources: SUCCEEDED is not a Square refund status. Square documents a settled refund as COMPLETED (PaymentRefund object) or APPROVED (current RefundStatus enum); SUCCEEDED appears in neither, so accepting it would record a contract-drift response as a confirmed refund. Removed SUCCEEDED from CONFIRMED_REFUND_STATUSES (now APPROVED and COMPLETED only, both Square-documented across versions); the SUCCEEDED test now asserts false (treated as contract drift). This matches the job intent (accept only confirmed Square refund outcomes) and is the inverse of adding APPROVED: align the allow-list with Square documentation rather than the literal job spec, which predated Square current enum. Exhaustive mutation on square.ts is 100% (299 killed, 3 equivalents suppressed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76e79fc792
ℹ️ 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".
…review)
Codex P2: the malformed-response guard checked typeof refund.id === string but not non-empty, so a malformed refund: { id: "", status: "APPROVED" } would have been recorded as confirmed (an empty id is not a usable provider refund reference). Added a non-empty id check (and an empty-id regression test). Also resynced the refundPayment docstring — it still listed SUCCEEDED after that status was dropped from CONFIRMED_REFUND_STATUSES; it now names only APPROVED and COMPLETED and the malformed-shape clause covers missing, empty, and non-string. The two ~400-line test-file splits Codex flagged are recorded in TODO.md (pure test-organisation, separate from the refund contract). Exhaustive mutation on square.ts is 100% (302 killed, 3 equivalents suppressed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c0bebdec8
ℹ️ 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".
…ean TODOs (PR #1911 review) Codex P1-1 (regression test): added a test that creates a quantity-0-only placeholder attendee with a payment leg (no sale, no refund_cash), stubs isPaymentRefunded to return true, calls the refresh-payment endpoint, and verifies the refund_cash leg was posted (ledger reconciled). Also extracted a postPaymentLeg helper to dedup the mapBooking/postTransfers pattern. Codex P1-2 (stale manual-refund note): handleRefreshPayment now adds a resolving system note ('Refund confirmed: the payment provider reported this refund as settled. No manual refund is needed.') when the refresh confirms and records the refund, so the operator does not follow the stale 'refund manually' instruction left by storeRefundedBooking. Codex P1-3 (stale TODOs): removed the completed 'Validate Square REST responses' and 'Split the Square refund tests' TODO entries. Replaced with a single note about the remaining orders/payments validation work; the refund validation and test splits are done.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba77561040
ℹ️ 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".
…ing test (PR #1911 review) Codex P1 (restrict balance bypass): isPaymentOnlyPlaceholder now checks every leg is a provider-payment leg (no sale/fee/adjustment), so a surcharge-only order (payment + fee, no sale) is NOT misclassified as a placeholder and the balance guard is retained for it. Codex P2 (limit resolving note): the system note is now only created for a quantity-0-only placeholder (no quantity > 0 booking), not for every refreshed refund. Also moved the text to the locale catalog (attendees.json: note.placeholder_refund_confirmed) via t(). Extracted recordConfirmedRefund helper to reduce handleRefreshPayment cognitive complexity. Codex P1 (deleted-listing test): added a test that deletes the listing before refreshing, proving the loadRefreshContext fallback (using listing_id from the booking row, not requireListingWithCount) works for deleted-listing placeholders. Extracted setupPlaceholderForRefresh + refreshAndVerifyRefundCash helpers to dedup the two placeholder tests.
Codex follow-up (thread 3): the boundary throw lived inside withClient, which catches non-PaymentUserError exceptions and returns null, so it was architecturally the same as a logged false — an indirect throw-and-catch rather than a real loud failure. Replaced it with an explicit logError + return false, matching the missing-amount case just above it: the malformed response is still diagnosed at the boundary under E_SQUARE_REFUND and still fails safely to false (never reports a refund that was never confirmed), now without the swallowed-throw indirection. Propagating the throw outside withClient (a loud/errored failure) would change the contract from fail-safely-contained to fail-loudly, which is the per-path staged-checkout/callback resolution this PR was told not to introduce and is recorded in TODO.md against #1853 and #1905. Exhaustive mutation on square.ts is now 100% (286 killed, 3 genuine equivalents suppressed).
CodeRabbit and Codex flagged that Square's RefundStatus enum includes APPROVED (the current terminal success state for a refund), which CONFIRMED_REFUND_STATUSES omitted — a refund Square approved would have been wrongly treated as not-confirmed. Added APPROVED alongside COMPLETED and SUCCEEDED (kept per the job spec as defensive aliases), with a regression test. Confirmed APPROVED against Square's current API reference. Also corrected the now-stale comment in the malformed-response test (it no longer throws; it logs under E_SQUARE_REFUND and returns false). Exhaustive mutation on square.ts is still 100% (288 killed, 3 genuine equivalents suppressed; line numbers refreshed).
… rebase After rebasing onto main (absorbing #1909 test hardening, #1910 equivalent-registry cleanup, and #1912 stable Square refund idempotency keys), the three square.ts equivalent-mutant entries moved to 294/684/754 and the Square refundPayment now uses #1912 stable refundIdempotencyKey instead of crypto.randomUUID. Updated the entries to match and the TODO note for #1912 landing (the stable key mitigates the double-pay half of the PENDING-refund redelivery risk). Exhaustive mutation on square.ts is 100% (290 killed, 3 equivalents suppressed).
…iew) Codex follow-ups: the refundPayment docstring still listed only COMPLETED/SUCCEEDED after APPROVED was added (now references CONFIRMED_REFUND_STATUSES), and the malformed-response guard only checked truthiness, so a truthy non-string id (e.g. id: 1) with an allow-listed status would have been recorded as a confirmed refund — the guard now requires string id/status (a missing or non-string field logs at the boundary under E_SQUARE_REFUND and returns false). Exhaustive mutation on square.ts is 100% (301 killed, 3 equivalents suppressed).
…#1911 review) Codex P2 with Square doc sources: SUCCEEDED is not a Square refund status. Square documents a settled refund as COMPLETED (PaymentRefund object) or APPROVED (current RefundStatus enum); SUCCEEDED appears in neither, so accepting it would record a contract-drift response as a confirmed refund. Removed SUCCEEDED from CONFIRMED_REFUND_STATUSES (now APPROVED and COMPLETED only, both Square-documented across versions); the SUCCEEDED test now asserts false (treated as contract drift). This matches the job intent (accept only confirmed Square refund outcomes) and is the inverse of adding APPROVED: align the allow-list with Square documentation rather than the literal job spec, which predated Square current enum. Exhaustive mutation on square.ts is 100% (299 killed, 3 equivalents suppressed).
…review)
Codex P2: the malformed-response guard checked typeof refund.id === string but not non-empty, so a malformed refund: { id: "", status: "APPROVED" } would have been recorded as confirmed (an empty id is not a usable provider refund reference). Added a non-empty id check (and an empty-id regression test). Also resynced the refundPayment docstring — it still listed SUCCEEDED after that status was dropped from CONFIRMED_REFUND_STATUSES; it now names only APPROVED and COMPLETED and the malformed-shape clause covers missing, empty, and non-string. The two ~400-line test-file splits Codex flagged are recorded in TODO.md (pure test-organisation, separate from the refund contract). Exhaustive mutation on square.ts is 100% (302 killed, 3 equivalents suppressed).
Blocker 1 (PENDING placeholder stranding): traced store-refund.ts → refund-ledger.ts → attendees-edit.ts. A PENDING Square refund returns false from refundPayment, which made storeRefundedBooking terminal with only the payment leg (no refund_cash). The operator could not refresh because loadRefreshContext filtered quantity > 0 (excluding placeholders), and computeAttendeeRefund skipped them as unbalanced. Fixed: loadRefreshContext now prefers quantity > 0 but falls back to quantity-0 placeholders (ORDER BY (quantity > 0) DESC); computeAttendeeRefund now allows reversal for placeholder accounts (no sale legs) regardless of balance, so a settled PENDING refund can be reconciled via the refresh-payment path. Also dropped the ListingWithCount requirement (uses listing_id from the booking row, so a deleted listing does not 404 the refresh). Blocker 2 (APPROVED): verified against the Square PaymentRefund object documentation — its status field is PENDING/COMPLETED/REJECTED/FAILED (NOT APPROVED). APPROVED is only in the separate RefundStatus enum page, not the PaymentRefund object that /v2/refunds returns. Removed APPROVED; CONFIRMED_REFUND_STATUSES is now [COMPLETED] only. Blocker 3 (Valibot boundary): replaced the type cast with a Valibot schema (SquareRefundSchema with v.string() + v.minLength(1) for id and status, matching Square docs: id is Required with Min Length 1). v.parse runs OUTSIDE withClient so a malformed response (missing refund, empty id/status, non-string fields) throws loudly instead of normalizing to false. The client returns raw unknown; the boundary validation is in squareApi.refundPayment. Also split retrieve-refund.test.ts (481→240) and rest-transport.test.ts (429→372) by extracting refund-payment.test.ts (247 lines) and refund-transport.test.ts (123 lines) with a shared mock-fetch helper. Added minLength boundary tests and a REJECTED status test. Exhaustive mutation on square.ts is 100% (284 killed, 2 equivalents suppressed).
…ean TODOs (PR #1911 review) Codex P1-1 (regression test): added a test that creates a quantity-0-only placeholder attendee with a payment leg (no sale, no refund_cash), stubs isPaymentRefunded to return true, calls the refresh-payment endpoint, and verifies the refund_cash leg was posted (ledger reconciled). Also extracted a postPaymentLeg helper to dedup the mapBooking/postTransfers pattern. Codex P1-2 (stale manual-refund note): handleRefreshPayment now adds a resolving system note ('Refund confirmed: the payment provider reported this refund as settled. No manual refund is needed.') when the refresh confirms and records the refund, so the operator does not follow the stale 'refund manually' instruction left by storeRefundedBooking. Codex P1-3 (stale TODOs): removed the completed 'Validate Square REST responses' and 'Split the Square refund tests' TODO entries. Replaced with a single note about the remaining orders/payments validation work; the refund validation and test splits are done.
…ing test (PR #1911 review) Codex P1 (restrict balance bypass): isPaymentOnlyPlaceholder now checks every leg is a provider-payment leg (no sale/fee/adjustment), so a surcharge-only order (payment + fee, no sale) is NOT misclassified as a placeholder and the balance guard is retained for it. Codex P2 (limit resolving note): the system note is now only created for a quantity-0-only placeholder (no quantity > 0 booking), not for every refreshed refund. Also moved the text to the locale catalog (attendees.json: note.placeholder_refund_confirmed) via t(). Extracted recordConfirmedRefund helper to reduce handleRefreshPayment cognitive complexity. Codex P1 (deleted-listing test): added a test that deletes the listing before refreshing, proving the loadRefreshContext fallback (using listing_id from the booking row, not requireListingWithCount) works for deleted-listing placeholders. Extracted setupPlaceholderForRefresh + refreshAndVerifyRefundCash helpers to dedup the two placeholder tests.
…1911 review) Codex P2 (picklist): the Valibot schema now validates status with v.picklist([PENDING, COMPLETED, REJECTED, FAILED]) — the four documented PaymentRefund statuses. An undocumented status (APPROVED, SUCCEEDED, WAT, etc.) throws at the boundary instead of silently becoming an ordinary false outcome. Updated tests: undocumented statuses now expect a throw via expectMalformedThrows. Codex P1 (resolve warning via ledger): recordConfirmedRefund now checks the ledger (transfersByAccount + KIND.sale) for whether the attendee has sale legs, not the booking quantities. If the operator added a real booking to a placeholder attendee before the refund settled, the ledger would show sale legs and the resolving note would NOT be created (correctly), because the original manual-refund warning from storeRefundedBooking only fires for quantity-0-only placeholders with no sale. The ledger is the authoritative check.
… (PR #1911 review) Codex P2 (docstring): the refundPayment docstring now distinguishes documented non-success statuses (PENDING/REJECTED/FAILED → false) from undocumented/malformed statuses (not in the picklist → throws), reflecting the v.picklist validation. Codex P2 (stale TODO line numbers): removed the concrete line numbers (293, 678, 748) from the stale-equivalent-mutants TODO section — they drift as the source shifts lines. The text now references the audit command's own output (which lists every stale file:line:col) without embedding numbers that immediately go stale.
…#1911 review) Codex P1 (deleted-listing test): the test callback now receives the actual listing ID (not a hardcoded 0), so it deletes the correct listing row. The test comment clarifies that deleteListing would cascade to the attendee — this test exercises the case where only the listing row is gone (the real-world scenario for a storeRefundedBooking placeholder whose listing was already removed). Codex P2 (surcharge-only order): the placeholder check now uses legMatches({ from: WORLD, kind: KIND.payment }) (via sameAccount) instead of === (which compared object references for AccountRef). A surcharge-only order (payment + fee, no sale) is NOT classified as a placeholder because the fee leg is not a provider-payment leg.
Codex P2: refundPayment now gets the Square client directly (not through withClient) so a successful HTTP 200 response — even a null or invalid-JSON body — always reaches the Valibot parse. withClient would catch a JSON.parse throw or a JSON null return and normalize to null → false. With the direct approach: client-unconfigured returns false; HTTP/network/JSON errors are caught by a targeted try/catch (logged → false, retryable); a 200 body always reaches v.parse (malformed → throws loudly). Added a test for the client-unavailable-after-retrieval edge case.
Codex P2: the try/catch now re-throws SyntaxError (JSON.parse failure from a 200 body) so invalid JSON in a successful response propagates — it fails loudly at the boundary instead of being caught as a generic error → false. HTTP errors and network errors still return false (graceful, retryable). Added a test: a mock rejecting with SyntaxError causes refundPayment to reject (not resolve to false).
…review) Codex P2: the Valibot schema now requires payment_id (non-empty string) and amount_money (non-negative number + non-empty currency) on the PaymentRefund object, and refundPayment verifies the refund's payment_id matches the paymentId it was called with — a COMPLETED refund for a different payment throws loudly. Updated all test mocks to include the required fields. Added a test for the wrong-payment-id case.
…alformed throws (PR #1911 review) Codex P2: the docstring now correctly documents three failure modes: (1) HTTP errors and network failures → logged and returned as false (graceful, retryable); (2) a 200 response is always validated by v.parse — a malformed body throws loudly; (3) client-unconfigured returns false. The previous docstring incorrectly said HTTP and parsing errors propagate.
…te; fix coverage E2e: added afterPaidBooking hook to the Square provider using the shared exerciseAdminRefund helper — the nightly Square sandbox run now exercises the real POST /v2/refunds, the Valibot boundary schema, the COMPLETED status check, the payment_id/amount verification, and the ledger posting. Extracted the shared admin refund exercise from exerciseStripeRefund into shared.ts so Stripe and Square share one implementation. Validation: the Valibot schema now requires payment_id and amount_money on the PaymentRefund object; refundPayment verifies the refund is for the correct payment AND the amount matches before accepting COMPLETED. Added tests for wrong-payment-id, wrong-amount, and wrong-currency (all throw). Note cleanup: recordConfirmedRefund deletes the stale 'could NOT be refunded' system note from storeRefundedBooking before creating the resolution note. Added a test: creates the stale note, refreshes, verifies the stale note is deleted and the confirmation note is present. Equivalent-mutant line numbers refreshed (312/783). Deduped the getRefundPaymentReferences call pattern via getRefundPaymentReferencesForAttendee.
90aa5d3 to
99d3860
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99d3860b42
ℹ️ 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".
| ORDER BY (listingAttendee.quantity > 0) DESC, | ||
| listingAttendee.start_at, listingAttendee.listing_id | ||
| LIMIT 1`, |
There was a problem hiding this comment.
When the placeholder's listing is deleted through the application, deleteListing removes its listing_attendees rows before deleting the listing (src/shared/db/listings/delete.ts:8-13), so this booking-row query returns nothing and the refresh route responds with 404. A Square refund that settles afterward therefore cannot be recorded, leaving the stale manual-refund warning and risking another refund attempt. Fresh evidence after the earlier thread is that the final regression test explicitly avoids deleteListing and directly deletes only the listing row, preserving the condition this query requires; load the context without requiring a booking row and cover the real deletion path.
AGENTS.md reference: AGENTS.md:L83-L83
Useful? React with 👍 / 👎.
| const isPlaceholder = | ||
| legsBeforeRefund.length > 0 && legsBeforeRefund.every(isProviderPayment); |
There was a problem hiding this comment.
When a pending-refund placeholder later gains a paid booking before all refunds settle—for example, by keeping the placeholder as the target of an attendee merge—its account contains sale and payment legs, so this every(isProviderPayment) check is false. Once every provider payment is refunded, recordAttendeeRefund can still reverse the fully paid account successfully, but this branch leaves the original red note telling the operator to refund manually, risking a second refund. Fresh evidence after the earlier thread is that the final code gates warning deletion on the account still being payment-only rather than on the warning's existence; delete the matching warning after confirmed recovery regardless of later account shape and cover that exact sequence.
AGENTS.md reference: AGENTS.md:L83-L83
Useful? React with 👍 / 👎.
| await requirePageText( | ||
| session, | ||
| "Refund issued", | ||
| "refund-failed", | ||
| 'Expected the app page to contain "Refund issued"', |
There was a problem hiding this comment.
When the Square sandbox returns the documented PENDING outcome, this assertion fails even though the refund request was accepted: squareApi.refundPayment now returns false for that status, and the inspected admin path in src/features/admin/refunds/provider.ts:39-56 performs only one immediate isPaymentRefunded check before rendering “Refund failed.” The new Square afterPaidBooking hook therefore makes the nightly journey depend on the refund becoming COMPLETED synchronously; poll through the refresh flow until the accepted refund settles before requiring the success flash.
Useful? React with 👍 / 👎.
…; add e2e Square refund Split test/features/admin/attendees-edit.test.ts (410→297 lines) by extracting the placeholder refresh tests into test/features/admin/placeholder-refresh.test.ts (115 lines). Both files now stay under the ~400-line target. Extracted the shared postPaymentLeg helper into test/test-utils/db-helpers/payment-leg.ts. E2e: the nightly Square sandbox run now exercises the real refund path (afterPaidBooking → exerciseAdminRefund) via the shared shared.ts helper, so the POST /v2/refunds → Valibot schema → COMPLETED check → payment_id/amount verification → ledger posting round-trip is exercised against the real Square sandbox API every night. Extracted exerciseAdminRefund into shared.ts so Stripe and Square share one implementation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23cae697ce
ℹ️ 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".
…1911 review) Codex P1: the stale-note cleanup now runs on every successful refresh, regardless of whether attendee.refunded was already true. Previously, if the note cleanup failed after the ledger posted, attendee.refunded became true and the next refresh skipped recordConfirmedRefund entirely (because of the !attendee.refunded condition), stranding the stale manual-refund note forever. Fixed by: (1) always calling recordConfirmedRefund when all references are provider-refunded (the ledger post is idempotent — already-posted returns {posted: true} as a no-op); (2) moving cleanupStaleManualRefundNote outside the isPlaceholder check (after the first refresh, the account has a refund_cash leg so isPlaceholder is false, but the stale note could still be there); (3) extracted loadRefreshState to keep handleRefreshPayment under cognitive complexity 12. Added a test: first refresh posts the ledger, re-creates the stale note, second refresh cleans it up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d549c0b11f
ℹ️ 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 P2: the activity log ('Payment marked as refunded') now only fires when attendee.refunded was false before the refresh. On retry when the ledger was already posted, the activity log is skipped (no duplicate audit entries), but the stale-note cleanup still runs unconditionally.
1 participant
Square refunds previously reported success the moment the POST to
/v2/refundsreturned an OK response, even when the refund was stillPENDING. That counted a half-finished refund as done. This PR makes the contract honest and addresses all three merge blockers from the independent review.What changed
refundPaymentnow parses the refund object Square returns through a Valibot boundary schema and reports success only for a Square-documentedCOMPLETEDstatus:true(the money has been returned to the buyer).false.id/status→ throws loudly (Valibotv.parseruns outsidewithClient, so the malformed response is visible instead of silently normalized tofalse). The schema matches Square's own PaymentRefund docs:idis Required with Min Length 1,statusis a string.APPROVEDwas removed — it is in Square's RefundStatus enum page but NOT the PaymentRefund object documentation (which lists PENDING/COMPLETED/REJECTED/FAILED).SUCCEEDEDwas also removed (not a Square status).Blocker 1 — PENDING placeholder stranding (fixed, not deferred)
Traced
store-refund.ts→refund-ledger.ts→attendees-edit.ts: a PENDING refund returnedfalse, which madestoreRefundedBookingterminal with only the payment leg (norefund_cash). The operator could not refresh becauseloadRefreshContextfilteredquantity > 0(excluding quantity-0 placeholders), andcomputeAttendeeRefundskipped them as unbalanced. Fixed:loadRefreshContext: NOW prefersquantity > 0but falls back to quantity-0 placeholders viaORDER BY (quantity > 0) DESC— so a placeholder can be refreshed. Useslisting_idfrom the booking row (norequireListingWithCountcall), so a deleted listing does not 404 the refresh.computeAttendeeRefund: NOW allows reversal for placeholder accounts (payment legs only, no sale legs) regardless of balance — the refund_cash leg simply returns the payment, balance zeroes cleanly.handleRefreshPayment: refreshed to uselistingId(from the booking) instead of the full listing object.When a PENDING Square refund later settles (COMPLETED), the operator clicks "refresh payment" →
isPaymentRefundedreturns true (money moved) →recordAttendeeRefund→computeAttendeeRefunddetects the placeholder (no sale legs) → posts therefund_cashleg → ledger reconciled.Stable idempotency + downstream
Uses
refundIdempotencyKey("square", paymentId)from #1912 (the stable-refund-keys sibling job). Downstream, afalseis handled by the existingtryRefundfallback (isPaymentRefunded); the stable key prevents a double payout on redelivery. The PENDING-as-pending union (carrying the refund id instead of collapsing tofalse) is the staged-checkout/callback work recorded inTODO.mdagainst #1853 and #1905.Tests
Split
retrieve-refund.test.ts(481→240 lines) andrest-transport.test.ts(429→372) by extracting:refund-payment.test.ts(247 lines) — status matrix (COMPLETED→true, PENDING/REJECTED/FAILED/APPROVED/SUCCEEDED/unknown→false), Valibot boundary throws (missing refund, empty id/status, non-string fields, minLength boundary cases).refund-transport.test.ts(123 lines) — raw response return for the boundary validator.test/lib/square/mock-fetch.ts— sharedjsonResponse/installMockFetchhelper.Exhaustive mutation on
square.tsis 100% (284 killed, 2 genuine equivalents suppressed). Verified withnix develop -c deno task precommit(typecheck, lint, cpd, build, full test suite) on a branch rebased onto main (#1909/#1910/#1912).