fix(credits): stop minting a Checkout Session on every credit-gated 402 - #836
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (13)
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 |
There was a problem hiding this comment.
3 issues found across 38 files
Confidence score: 3/5
- In
lib/stripe/stripeWebhookHandler.ts, the/api/credits/sessionssaved-card off-session success path no longer sends the PaymentIntent sales notification, so some successful top-ups could become invisible to sales/ops workflows—restore the PaymentIntent notification import and call in that branch. - In
lib/stripe/__tests__/stripeWebhookHandler.test.ts, the renamedpayment_intent.succeededtest appears fully duplicated by another test, which adds noise and can hide true coverage gaps over time—remove or refocus it on a distinct behavior. - In
lib/credits/__tests__/buildInsufficientCreditsResponse.test.ts, hardcoding"https://chat.recoupable.dev"instead of usingCREDIT_BILLING_URLcan cause brittle failures or miss config regressions whenCHAT_APP_URLchanges—assert against the imported constant to keep the test aligned with runtime configuration.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/stripe/__tests__/stripeWebhookHandler.test.ts">
<violation number="1" location="lib/stripe/__tests__/stripeWebhookHandler.test.ts:165">
P3: The renamed "grants credits on payment_intent.succeeded" test is now redundant: its only assertion (that `processCreditsTopupPaymentIntentMock` is called with the payment intent) is fully covered by the existing "delegates payment_intent.succeeded to processCreditsTopupPaymentIntent" test. Since removing the notify assertion left this test without any unique coverage, drop this block to avoid two identical tests.</violation>
</file>
<file name="lib/stripe/stripeWebhookHandler.ts">
<violation number="1" location="lib/stripe/stripeWebhookHandler.ts:29">
P2: When `/api/credits/sessions` succeeds through a saved-card off-session charge, this deletion removes the only sales notification for that top-up. Restore the PaymentIntent notification import and call; keep the credit grant and notification as separate webhook steps.</violation>
</file>
<file name="lib/credits/__tests__/buildInsufficientCreditsResponse.test.ts">
<violation number="1" location="lib/credits/__tests__/buildInsufficientCreditsResponse.test.ts:40">
P3: The final test hardcodes the URL literal `"https://chat.recoupable.dev"` instead of comparing against the imported `CREDIT_BILLING_URL`. If `CHAT_APP_URL` moves (the PR's stated reason for deriving `CREDIT_BILLING_URL` from it), this assertion breaks and must be updated in a second place. Compare against `CREDIT_BILLING_URL` and assert URL-shape constraints separately so the test stays coupled to the single source of truth.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as API Client / open-agents
participant Route as Credit-Gated Route
participant Gate as ensureCreditsOrShortCircuit
participant Check as checkCreditsAvailable
participant DB as Supabase credits_usage
participant Stripe as Stripe API
participant Sessions as POST /api/credits/sessions
Note over Client,Sessions: Credit Gate Flow - No Stripe Objects on 402
Client->>Route: Request (chat, research, socials)
Route->>Gate: ensureCreditsOrShortCircuit({accountId, creditsToDeduct})
Gate->>Check: checkCreditsAvailable({accountId, creditsToDeduct})
Check->>DB: selectCreditsUsage()
DB-->>Check: remaining_credits
alt Sufficient credits (remaining >= cost)
Check-->>Gate: {kind: "available"}
Gate-->>Route: null
Route-->>Client: Proceed with work
else Insufficient credits (remaining < cost)
Check-->>Gate: {kind: "insufficient_credits", remaining, required}
Note over Gate: No Stripe customer resolution<br/>No Checkout Session minted<br/>No off-session charge
Gate->>Gate: buildInsufficientCreditsResponse()
Gate-->>Route: 402 NextResponse
Note right of Gate: Body: {error, remaining_credits,<br/>required_credits, billingUrl}
Route-->>Client: 402 + billingUrl (static CHAT_APP_URL)
end
Note over Client,Sessions: Manual Top-Up - Only Place Sessions Are Minted
Client->>Sessions: POST /api/credits/sessions
Sessions->>Stripe: createCreditsStripeSession()
Stripe-->>Sessions: {id, url, declineReason?}
Sessions-->>Client: {id, url, declineReason?}
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| await notifyCreditsTopupSession(event.data.object as Stripe.Checkout.Session); | ||
| } else if (event.type === "payment_intent.succeeded") { | ||
| await processCreditsTopupPaymentIntent(event.data.object as Stripe.PaymentIntent); | ||
| await notifyCreditsTopupPaymentIntent(event.data.object as Stripe.PaymentIntent); |
There was a problem hiding this comment.
P2: When /api/credits/sessions succeeds through a saved-card off-session charge, this deletion removes the only sales notification for that top-up. Restore the PaymentIntent notification import and call; keep the credit grant and notification as separate webhook steps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/stripe/stripeWebhookHandler.ts, line 29:
<comment>When `/api/credits/sessions` succeeds through a saved-card off-session charge, this deletion removes the only sales notification for that top-up. Restore the PaymentIntent notification import and call; keep the credit grant and notification as separate webhook steps.</comment>
<file context>
@@ -26,7 +25,6 @@ export async function stripeWebhookHandler(request: NextRequest): Promise<NextRe
await processCreditsTopupPaymentIntent(event.data.object as Stripe.PaymentIntent);
- await notifyCreditsTopupPaymentIntent(event.data.object as Stripe.PaymentIntent);
} else if (event.type === "invoice.paid") {
await processInvoicePaid(event.data.object as Stripe.Invoice);
} else if (event.type === "customer.subscription.created") {
</file context>
|
|
||
| it("notifies alongside the credits grant on payment_intent.succeeded", async () => { | ||
| const pi = { id: "pi_1", metadata: { purpose: "credits_auto_recharge" } }; | ||
| it("grants credits on payment_intent.succeeded", async () => { |
There was a problem hiding this comment.
P3: The renamed "grants credits on payment_intent.succeeded" test is now redundant: its only assertion (that processCreditsTopupPaymentIntentMock is called with the payment intent) is fully covered by the existing "delegates payment_intent.succeeded to processCreditsTopupPaymentIntent" test. Since removing the notify assertion left this test without any unique coverage, drop this block to avoid two identical tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/stripe/__tests__/stripeWebhookHandler.test.ts, line 165:
<comment>The renamed "grants credits on payment_intent.succeeded" test is now redundant: its only assertion (that `processCreditsTopupPaymentIntentMock` is called with the payment intent) is fully covered by the existing "delegates payment_intent.succeeded to processCreditsTopupPaymentIntent" test. Since removing the notify assertion left this test without any unique coverage, drop this block to avoid two identical tests.</comment>
<file context>
@@ -167,14 +162,13 @@ describe("stripeWebhookHandler", () => {
- it("notifies alongside the credits grant on payment_intent.succeeded", async () => {
- const pi = { id: "pi_1", metadata: { purpose: "credits_auto_recharge" } };
+ it("grants credits on payment_intent.succeeded", async () => {
+ const pi = { id: "pi_1", metadata: { purpose: "credits_topup" } };
verifyStripeWebhookEventMock.mockResolvedValue({
</file context>
| requiredCredits: 1, | ||
| }); | ||
|
|
||
| expect(billingUrl).toBe("https://chat.recoupable.dev"); |
There was a problem hiding this comment.
P3: The final test hardcodes the URL literal "https://chat.recoupable.dev" instead of comparing against the imported CREDIT_BILLING_URL. If CHAT_APP_URL moves (the PR's stated reason for deriving CREDIT_BILLING_URL from it), this assertion breaks and must be updated in a second place. Compare against CREDIT_BILLING_URL and assert URL-shape constraints separately so the test stays coupled to the single source of truth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/credits/__tests__/buildInsufficientCreditsResponse.test.ts, line 40:
<comment>The final test hardcodes the URL literal `"https://chat.recoupable.dev"` instead of comparing against the imported `CREDIT_BILLING_URL`. If `CHAT_APP_URL` moves (the PR's stated reason for deriving `CREDIT_BILLING_URL` from it), this assertion breaks and must be updated in a second place. Compare against `CREDIT_BILLING_URL` and assert URL-shape constraints separately so the test stays coupled to the single source of truth.</comment>
<file context>
@@ -0,0 +1,45 @@
+ requiredCredits: 1,
+ });
+
+ expect(billingUrl).toBe("https://chat.recoupable.dev");
+ expect(billingUrl).not.toContain("stripe");
+ // The URL it replaces defaulted to sandbox.recoupable.com, the wrong host.
</file context>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
49b0489 to
d96250e
Compare
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
The credit gate called createCreditsStripeSession on both of its exit paths purely to populate checkoutUrl in the 402 body, so every scheduled task that ran out of credits minted a live Stripe Checkout Session, unattended and forever. 2,779 of them across 46 accounts in 30 days, none paid (recoupable/chat#1960). Removing the off-session charge did not fix this on its own. - checkCreditsAvailable drops the session minting, the Stripe customer resolution and the successUrl parameter. It now reads the balance and returns, making no Stripe calls on any path. - buildInsufficientCreditsResponse returns a static billingUrl (CREDIT_BILLING_URL = CHAT_APP_URL) in place of checkoutUrl. Retrying the gate returns the same URL and creates nothing. - successUrl comes off ensureCreditsOrShortCircuit and all eight credit-gated call sites. CREDIT_SHORTFALL_TOPUP_CREDITS and CREDIT_SHORTFALL_SUCCESS_URL are deleted; the latter also pointed at sandbox.recoupable.com, a host we do not serve, so some of those sessions would have returned users to the wrong place anyway. A real Checkout Session is still minted where someone asked for one, at POST /api/credits/sessions. That path keeps declineReason and is untouched, so open-agents' credits-topup dialog keeps working. ensureSongstatsPaymentMethod still mints a session per denied request. It is a payment-method gate rather than a credit gate, out of scope here, and now carries a comment saying so.
d96250e to
95e9bd7
Compare
Preview verification — the issue's exact Done-when, against live StripeRebased onto Documented vs actual
For contrast: the identical scenario on the #835 preview (before this change) minted one Conflict-resolution noteThe branch was stacked on #835; after that squash-merged, this PR was rebased with Reminder for mergeThe docs merged in docs#299 show Ready to merge. |
The credit gate's 402 body changed in recoupable/api#836 (chat#1960): billingUrl is a static app link and no Stripe object is created. Two example references still named checkoutUrl.
Implements the second defect in recoupable/chat#1960. Row 4 of that issue's PR matrix.
The credit gate called
createCreditsStripeSessionon both exit paths (L57-62 for opted-out, L98-103 as the fallback) purely to populatecheckoutUrlin the 402 body. So every credit-gated request that came up short minted a live Stripe Checkout Session, whether or not a human was there to see it.Measured on prod for the 30 days to 2026-08-17: 2,779 sessions, 46 accounts, 0 with a
customer_email, 0 paid. They cluster on cron minutes (244 at 06:01, 206 at 06:02, 117 at 09:03), which is scheduled-task traffic hitting the gate, not people. Removing the off-session charge in #835 does not fix this on its own.What changes
checkCreditsAvailableloses the session minting, the Stripe customer resolution and thesuccessUrlparameter. It readsremaining_creditsand returns. It makes no Stripe calls on any path. 61 lines → 42.buildInsufficientCreditsResponsereturns a staticbillingUrlin place ofcheckoutUrl:{ "error": "insufficient_credits", "remaining_credits": 12, "required_credits": 100, "billingUrl": "https://chat.recoupable.dev" }CREDIT_BILLING_URLisCHAT_APP_URL. Deliberately the app root rather than a deep link, so the app can move where a card is saved without changing this contract.successUrlis gone fromensureCreditsOrShortCircuitand all eight credit-gated call sites (lib/chat/validateChatRequest.ts,lib/socials/ensureSocialScrapeCredits.ts, and six underlib/research/).CREDIT_SHORTFALL_TOPUP_CREDITSandCREDIT_SHORTFALL_SUCCESS_URLare deleted. The latter pointed atsandbox.recoupable.com, which is not a host we serve, so a share of those 2,779 sessions would have returned users to the wrong place even if anyone had paid one.POST /api/credits/sessionsis untouched. That is where someone asked for a session, so that is where a session is still minted, and it keepsdeclineReason.open-agents' credits-topup dialog parses that{ id, url, declineReason? }response, not the 402, so it keeps working. ThebuildInsufficientCreditsResponsedocstring now says the two shapes are deliberately different; claiming they were the same is what conflated them.Out of scope, but worth knowing
lib/research/measurement_jobs/ensureSongstatsPaymentMethod.tsstill mints acreateCardOnFileSessionon every denied request. It is a payment-method gate, not a credit gate, so it is outside this issue, but it is the same shape of problem and now carries a comment saying so. ItssuccessUrlis repointed atCREDIT_BILLING_URLrather than the dead sandbox host, since the constant it used is being deleted.Verification
TDD, red before green:
checkCreditsAvailable.test.tsand addedbuildInsufficientCreditsResponse.test.tsfor the new contract, including a loop that drives 20 consecutive shortfalls and assertscreateCreditsStripeSessionwas called 0 times, plus assertions thatbillingUrlis identical across calls and contains neitherstripenorsandbox. Ran them: RED, 6 failed (expected { … } to not have property "checkoutUrl",expected undefined to be 'https://chat.recoupable.dev', and 3×Cannot read properties of undefined (reading 'url')from the mocked session no longer being reached).ensureCreditsOrShortCircuit.test.tsfor the two-field signature, asserting the gate is called with exactly{ accountId, creditsToDeduct }.Full runs:
vitest run lib/credits lib/stripe lib/billing lib/research lib/socials lib/chat app/api/webhooks app/api/credits app/api/accountseslint lib/research lib/socials lib/chat lib/credits lib/stripetsc --noEmitgrep -rn checkoutUrl lib appoutside testsensureSongstatsPaymentMethod.ts, the out-of-scope gate aboveNot yet done: the issue's live Done-when check (drive a credit-gated endpoint to 402 twenty times on preview and confirm zero new Checkout Sessions on that account in Stripe, and that
open-agents' dialog still renders a decline againstPOST /api/credits/sessions). The 20-iteration unit test covers the code path but not the live Stripe account. Flagging rather than implying it.Merge order
docs#299 → chat#1961 → api#835 → this PR.
Summary by cubic
Stops creating Stripe Checkout Sessions on credit‑gated 402s. Previously the gate minted a live session (and could charge a saved card); now it only checks balance and returns a static
billingUrl, so unattended calls create nothing and no charge happens without consent.checkCreditsAvailableis read‑only. It drops Stripe calls and thesuccessUrlparameter.buildInsufficientCreditsResponsereturnsbillingUrland removescheckoutUrland anydeclineReason.ensureCreditsOrShortCircuitno longer acceptssuccessUrl; all call sites inlib/chat,lib/research, andlib/socialsare updated.CREDIT_SHORTFALL_*constants withCREDIT_BILLING_URL(https://app.recoupable.dev).POST /api/credits/sessionsis unchanged and remains the only place a Checkout Session is minted.ensureSongstatsPaymentMethodstill mints a per‑denial session (payment‑method gate) and now usesCREDIT_BILLING_URLfor its success URL.Migration
billingUrlin 402 bodies instead ofcheckoutUrl. Do not expectdeclineReasonon 402.successUrlargument from allensureCreditsOrShortCircuitcalls. No changes toPOST /api/credits/sessions.Written for commit 95e9bd7. Summary will update on new commits.