Repository navigation
fix(reset-pass): prevent double-refunding - #3124
Conversation
A Reset Pass purchase was refundable whenever its tier inventory held at least one pass, so with multiple purchases of the same tier every purchase could claim the same unredeemed pass — including one whose pass was already redeemed. Since refund bookkeeping (credit_refund row and inventory clawback) only lands via the charge.refunded webhook, several refunds could also be issued back-to-back against a single remaining pass within the webhook latency window. Attribute redemptions to the oldest un-refunded purchase first: a purchase is only refundable while it ranks within the newest `inventory` un-refunded purchases of its tier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YhmLC9ModCS2f7AZ9BYSb
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
changed the title
WalkthroughReset-pass self-refund eligibility now evaluates tier-specific inventory against newer unrefunded purchases, with deterministic ordering and refunded-purchase exclusion. The dispatcher passes all transactions to this logic, and tests cover partial inventory, refunds, and lite/pro tier boundaries. ChangesReset-pass refund eligibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/lib/self-refund.ts (1)
291-293: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefensively parse
createdAtto prevent potential runtime errors.The existing code in
computeSelfRefundEligibilitywrapstransaction.createdAtinnew Date()before calling.getTime()(see line 343). This implies that timestamps might occasionally be passed as serialized strings (e.g., from an API payload). Ift.createdAtis a string, calling.getTime()directly will crash the handler with aTypeError.🛠️ Proposed fix to parse dates defensively
- (t.createdAt > transaction.createdAt || - (t.createdAt.getTime() === transaction.createdAt.getTime() && - t.id > transaction.id)), + (new Date(t.createdAt).getTime() > new Date(transaction.createdAt).getTime() || + (new Date(t.createdAt).getTime() === new Date(transaction.createdAt).getTime() && + t.id > transaction.id)),🤖 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 `@apps/api/src/lib/self-refund.ts` around lines 291 - 293, Update the date comparison in computeSelfRefundEligibility to defensively normalize t.createdAt before calling getTime(), matching the existing transaction.createdAt parsing behavior. Preserve the ordering logic using the parsed timestamp and t.id as the tie-breaker.apps/api/src/lib/self-refund.spec.ts (1)
503-535: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a test verifying that incomplete refunds do not unblock older purchases.
To lock in the security fix regarding
isCompleted(t)for refunds, consider adding a sister test to this one that explicitly seeds acredit_refundwithstatus: "pending"or"failed"for the newer purchase. The test should assert that the older transaction remainsineligiblebecause the incomplete refund hasn't definitively clawed back inventory yet.🤖 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 `@apps/api/src/lib/self-refund.spec.ts` around lines 503 - 535, The refund eligibility tests around “a refunded newer purchase no longer blocks the older one” lack coverage for incomplete refunds. Add sister cases that seed the newer purchase’s credit_refund with pending and/or failed status, then assert getEligibility(older.id) returns ineligible because the refund has not completed; preserve the existing completed-refund test.
🤖 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 `@apps/api/src/lib/self-refund.ts`:
- Around line 279-283: Update the refundedIds construction near the existing
credit_refund filter to include only transactions for which isCompleted(t) is
true. Keep requiring type === "credit_refund" and relatedTransactionId, so
pending or failed refunds do not affect newerUnrefundedSameTier rank counting.
---
Nitpick comments:
In `@apps/api/src/lib/self-refund.spec.ts`:
- Around line 503-535: The refund eligibility tests around “a refunded newer
purchase no longer blocks the older one” lack coverage for incomplete refunds.
Add sister cases that seed the newer purchase’s credit_refund with pending
and/or failed status, then assert getEligibility(older.id) returns ineligible
because the refund has not completed; preserve the existing completed-refund
test.
In `@apps/api/src/lib/self-refund.ts`:
- Around line 291-293: Update the date comparison in
computeSelfRefundEligibility to defensively normalize t.createdAt before calling
getTime(), matching the existing transaction.createdAt parsing behavior.
Preserve the ordering logic using the parsed timestamp and t.id as the
tie-breaker.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8352c131-0ce8-49bb-a128-6bc397d921fc
📒 Files selected for processing (2)
apps/api/src/lib/self-refund.spec.tsapps/api/src/lib/self-refund.ts
| const refundedIds = new Set( | ||
| transactions | ||
| .filter((t) => t.type === "credit_refund" && t.relatedTransactionId) | ||
| .map((t) => t.relatedTransactionId), | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Require credit_refund transactions to be completed to prevent double-refunds.
If a newer pass has a pending or failed refund, its relatedTransactionId is added to refundedIds here. This causes the newer pass to be excluded from the newerUnrefundedSameTier count below, bypassing the rank check. Because an incomplete refund has not clawed back the inventory (and won't, if failed), this artificially lowers the rank count and allows older passes to be refunded against the same unredeemed inventory, creating a double-refund loophole.
Filter for isCompleted(t) to ensure rank count and inventory stay properly in sync.
🔒️ Proposed fix to require completed refunds
const refundedIds = new Set(
transactions
- .filter((t) => t.type === "credit_refund" && t.relatedTransactionId)
+ .filter((t) => t.type === "credit_refund" && isCompleted(t) && t.relatedTransactionId)
.map((t) => t.relatedTransactionId),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const refundedIds = new Set( | |
| transactions | |
| .filter((t) => t.type === "credit_refund" && t.relatedTransactionId) | |
| .map((t) => t.relatedTransactionId), | |
| ); | |
| const refundedIds = new Set( | |
| transactions | |
| .filter((t) => t.type === "credit_refund" && isCompleted(t) && t.relatedTransactionId) | |
| .map((t) => t.relatedTransactionId), | |
| ); |
🤖 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 `@apps/api/src/lib/self-refund.ts` around lines 279 - 283, Update the
refundedIds construction near the existing credit_refund filter to include only
transactions for which isCompleted(t) is true. Keep requiring type ===
"credit_refund" and relatedTransactionId, so pending or failed refunds do not
affect newerUnrefundedSameTier rank counting.
Sync with main's Reset Pass refunds (#3120/#3124): the reset-pass refunded-purchase attribution and the new admin devpass refund queries only matched credit_refund rows; match subscription_refund too since plan refunds are now recorded with that type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sync with main's Reset Pass refunds (#3120/#3124): the reset-pass refunded-purchase attribution and the new admin devpass refund queries only matched credit_refund rows; match subscription_refund too since plan refunds are now recorded with that type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Updates the Reset Pass refund eligibility logic to prevent multiple purchases from being refunded against the same unredeemed pass. Passes are now attributed to the oldest un-refunded purchase first, and a purchase is only refundable if it ranks within the newest
inventoryun-refunded purchases of its tier.Changes
Updated
checkResetPassEligibilityfunction to accept the full transaction list and compute eligibility based on purchase rank rather than just inventory countpass_already_usedif the count meets or exceeds available inventoryAdded comprehensive test coverage for the new behavior:
Implementation Details
The fix uses a rank-based approach: for each purchase, count how many newer un-refunded purchases of the same tier exist. If this count is >= available inventory, the purchase is ineligible. This handles both the steady-state case (preventing double-refunds) and the transient window before the
charge.refundedwebhook records a clawback.https://claude.ai/code/session_015YhmLC9ModCS2f7AZ9BYSb
Summary by CodeRabbit
Bug Fixes
Tests