Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+25
to
+53
| const handleFalsePositivePastDue = async ({ | ||
| ctx, | ||
| stripeSubscription, | ||
| autumnStatus, | ||
| }: { | ||
| ctx: StripeWebhookContext; | ||
| stripeSubscription: ExpandedStripeSubscription; | ||
| autumnStatus: CusProductStatus; | ||
| }): Promise<CusProductStatus> => { | ||
| if (!isStripeSubscriptionPastDue(stripeSubscription)) return autumnStatus; | ||
|
|
||
| // const latestInvoice = await get | ||
| const latestInvoice = await getStripeInvoice({ | ||
| stripeClient: ctx.stripeCli, | ||
| invoiceId: stripeSubscription.latest_invoice, | ||
| expand: [], | ||
| }); | ||
|
|
||
| const metadata = latestInvoice?.metadata; | ||
|
|
||
| if ( | ||
| metadata?.autumn_billing_update && | ||
| metadata?.autumn_invoice_mode !== "true" | ||
| ) { | ||
| return CusProductStatus.Active; | ||
| } | ||
|
|
||
| return autumnStatus; | ||
| }; |
Contributor
There was a problem hiding this comment.
False-positive suppression may hide genuine payment failures
The current handleFalsePositivePastDue logic returns CusProductStatus.Active whenever the latest invoice has autumn_billing_update: "true" and autumn_invoice_mode !== "true", regardless of whether the invoice was actually paid.
Consider the scenario:
- A customer upgrades their plan → Autumn creates a manual invoice with
autumn_billing_update: "true" - The payment fails (card declined) → Stripe marks the subscription as
past_due subscription.updatedfires →handleFalsePositivePastDuesees the metadata and returnsCusProductStatus.Active- The customer is incorrectly shown as
Activedespite having a failed payment
The intent is to avoid marking a subscription as past_due when the invoice is an Autumn-initiated upgrade invoice (not a recurring charge failure). However, the check also needs to confirm the invoice was paid. A safe fix would be to also verify the invoice status is "paid":
if (
metadata?.autumn_billing_update &&
metadata?.autumn_invoice_mode !== "true" &&
latestInvoice?.status === "paid"
) {
return CusProductStatus.Active;
}This way, if the upgrade invoice itself fails to pay, the genuine past_due status is preserved.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/external/stripe/webhookHandlers/handleStripeSubscriptionUpdated/tasks/syncCustomerProductStatus/syncCustomerProductStatus.ts
Line: 25-53
Comment:
**False-positive suppression may hide genuine payment failures**
The current `handleFalsePositivePastDue` logic returns `CusProductStatus.Active` whenever the latest invoice has `autumn_billing_update: "true"` and `autumn_invoice_mode !== "true"`, regardless of whether the invoice was actually paid.
Consider the scenario:
1. A customer upgrades their plan → Autumn creates a manual invoice with `autumn_billing_update: "true"`
2. The payment **fails** (card declined) → Stripe marks the subscription as `past_due`
3. `subscription.updated` fires → `handleFalsePositivePastDue` sees the metadata and returns `CusProductStatus.Active`
4. The customer is incorrectly shown as `Active` despite having a failed payment
The intent is to avoid marking a subscription as `past_due` when the invoice is an Autumn-initiated upgrade invoice (not a recurring charge failure). However, the check also needs to confirm the invoice was paid. A safe fix would be to also verify the invoice status is `"paid"`:
```typescript
if (
metadata?.autumn_billing_update &&
metadata?.autumn_invoice_mode !== "true" &&
latestInvoice?.status === "paid"
) {
return CusProductStatus.Active;
}
```
This way, if the upgrade invoice itself fails to pay, the genuine `past_due` status is preserved.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
23
to
+34
| const isCreateAction = stripeSubscriptionAction?.type === "create"; | ||
| if (isCreateAction) return false; | ||
| if (isCreateAction) { | ||
| const willCreateInvoiceEndOfCycle = willStripeSubscriptionInvoiceEndOfCycle( | ||
| { | ||
| ctx, | ||
| billingContext, | ||
| autumnBillingPlan, | ||
| }, | ||
| ); | ||
|
|
||
| return willCreateInvoiceEndOfCycle; | ||
| } |
Contributor
There was a problem hiding this comment.
Early return when isCreateAction is false
When isCreateAction is true but willCreateInvoiceEndOfCycle is false, the function falls through to the custom line items check and the no-subscription path. This could produce unexpected behavior when a subscription create action exists but no consumable end-of-cycle invoicing is needed — for example, if there are no custom line items and no existing subscription, the function uses lineItems total as the decision, which may or may not be the right default for a create action.
Consider returning early for the create action regardless of willCreateInvoiceEndOfCycle:
if (isCreateAction) {
const willCreateInvoiceEndOfCycle = willStripeSubscriptionInvoiceEndOfCycle({
ctx,
billingContext,
autumnBillingPlan,
});
return willCreateInvoiceEndOfCycle;
// If false, no manual invoice needed for a create action (subscription creates its own first invoice)
}If the intent is to also handle custom line items during a create action, that logic should be made explicit rather than falling through to the shared path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/internal/billing/v2/providers/stripe/utils/invoices/shouldCreateManualStripeInvoice.ts
Line: 23-34
Comment:
**Early return when `isCreateAction` is false**
When `isCreateAction` is `true` but `willCreateInvoiceEndOfCycle` is `false`, the function falls through to the custom line items check and the no-subscription path. This could produce unexpected behavior when a subscription create action exists but no consumable end-of-cycle invoicing is needed — for example, if there are no custom line items and no existing subscription, the function uses `lineItems` total as the decision, which may or may not be the right default for a create action.
Consider returning early for the create action regardless of `willCreateInvoiceEndOfCycle`:
```typescript
if (isCreateAction) {
const willCreateInvoiceEndOfCycle = willStripeSubscriptionInvoiceEndOfCycle({
ctx,
billingContext,
autumnBillingPlan,
});
return willCreateInvoiceEndOfCycle;
// If false, no manual invoice needed for a create action (subscription creates its own first invoice)
}
```
If the intent is to also handle custom line items during a create action, that logic should be made explicit rather than falling through to the shared path.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 34 files
Confidence score: 2/5
- There are concrete billing-behavior regressions with fairly high confidence, so merge risk is elevated rather than just cosmetic or cleanup-level.
- In
shared/utils/billingUtils/invoicingUtils/lineItemBuilders/fixedPriceToLineItem.ts, dropping the explicitfalsefallback fordiscountablecan make fixed-price charge lines discountable by default, which can directly change invoice totals. - In
server/src/external/stripe/webhookHandlers/handleStripeSubscriptionUpdated/tasks/syncCustomerProductStatus/syncCustomerProductStatus.ts, missing invoice payment-status validation inhandleFalsePositivePastDuecan incorrectly treat failed-paymentpast_duesubscriptions as active, creating product-access/revenue leakage risk. - Pay close attention to
shared/utils/billingUtils/invoicingUtils/lineItemBuilders/fixedPriceToLineItem.tsandserver/src/external/stripe/webhookHandlers/handleStripeSubscriptionUpdated/tasks/syncCustomerProductStatus/syncCustomerProductStatus.ts- both alter core billing/subscription state decisions with user-facing impact.
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="shared/utils/billingUtils/invoicingUtils/lineItemBuilders/fixedPriceToLineItem.ts">
<violation number="1" location="shared/utils/billingUtils/invoicingUtils/lineItemBuilders/fixedPriceToLineItem.ts:37">
P1: This drops the explicit `false` default for `discountable`, allowing `undefined` to flow into `buildLineItem` and changing fixed-price items to default discountable on charge lines. Restore the fallback to avoid unintended discount application behavior.</violation>
</file>
<file name="server/tests/integration/billing/attach/new-plan/attach-recurring-oneoff.test.ts">
<violation number="1" location="server/tests/integration/billing/attach/new-plan/attach-recurring-oneoff.test.ts:291">
P2: The new integration test has no behavioral assertions; it only logs the attach result, so regressions in billing behavior can pass undetected.</violation>
</file>
<file name="server/src/external/stripe/webhookHandlers/handleStripeSubscriptionUpdated/tasks/syncCustomerProductStatus/syncCustomerProductStatus.ts">
<violation number="1" location="server/src/external/stripe/webhookHandlers/handleStripeSubscriptionUpdated/tasks/syncCustomerProductStatus/syncCustomerProductStatus.ts:46">
P1: Missing invoice payment status check in `handleFalsePositivePastDue` — if an Autumn-initiated upgrade invoice fails to pay (e.g., card declined), Stripe will mark the subscription as `past_due`, but this logic will still override the status to `Active` because it only checks metadata without verifying the invoice was actually paid. This hides genuine payment failures. Add a check for `latestInvoice?.status === "paid"` to the condition.</violation>
</file>
<file name="server/src/internal/billing/v2/providers/stripe/utils/invoices/createInvoiceForBilling.ts">
<violation number="1" location="server/src/internal/billing/v2/providers/stripe/utils/invoices/createInvoiceForBilling.ts:19">
P2: This function duplicates the existing `stripeDiscountsToParams` in `utils/discounts/stripeDiscountsToParams.ts`. Consider reusing that function instead of duplicating the logic here.</violation>
</file>
<file name="server/src/internal/billing/v2/execute/executeDeferredBillingPlan.ts">
<violation number="1" location="server/src/internal/billing/v2/execute/executeDeferredBillingPlan.ts:63">
P2: Cache deletion fails to delete customer data when `customerId` falls back to an empty string.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const updatedContext: LineItemContext = { | ||
| ...context, | ||
| discountable: context.discountable ?? false, | ||
| discountable: context.discountable, |
Contributor
There was a problem hiding this comment.
P1: This drops the explicit false default for discountable, allowing undefined to flow into buildLineItem and changing fixed-price items to default discountable on charge lines. Restore the fallback to avoid unintended discount application behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At shared/utils/billingUtils/invoicingUtils/lineItemBuilders/fixedPriceToLineItem.ts, line 37:
<comment>This drops the explicit `false` default for `discountable`, allowing `undefined` to flow into `buildLineItem` and changing fixed-price items to default discountable on charge lines. Restore the fallback to avoid unintended discount application behavior.</comment>
<file context>
@@ -34,7 +34,7 @@ export const fixedPriceToLineItem = ({
const updatedContext: LineItemContext = {
...context,
- discountable: context.discountable ?? false,
+ discountable: context.discountable,
};
</file context>
Suggested change
| discountable: context.discountable, | |
| discountable: context.discountable ?? false, |
Comment on lines
+46
to
+47
| metadata?.autumn_billing_update && | ||
| metadata?.autumn_invoice_mode !== "true" |
Contributor
There was a problem hiding this comment.
P1: Missing invoice payment status check in handleFalsePositivePastDue — if an Autumn-initiated upgrade invoice fails to pay (e.g., card declined), Stripe will mark the subscription as past_due, but this logic will still override the status to Active because it only checks metadata without verifying the invoice was actually paid. This hides genuine payment failures. Add a check for latestInvoice?.status === "paid" to the condition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/external/stripe/webhookHandlers/handleStripeSubscriptionUpdated/tasks/syncCustomerProductStatus/syncCustomerProductStatus.ts, line 46:
<comment>Missing invoice payment status check in `handleFalsePositivePastDue` — if an Autumn-initiated upgrade invoice fails to pay (e.g., card declined), Stripe will mark the subscription as `past_due`, but this logic will still override the status to `Active` because it only checks metadata without verifying the invoice was actually paid. This hides genuine payment failures. Add a check for `latestInvoice?.status === "paid"` to the condition.</comment>
<file context>
@@ -16,6 +19,39 @@ import { trackCustomerProductUpdate } from "../../../common/trackCustomerProduct
+ const metadata = latestInvoice?.metadata;
+
+ if (
+ metadata?.autumn_billing_update &&
+ metadata?.autumn_invoice_mode !== "true"
+ ) {
</file context>
Suggested change
| metadata?.autumn_billing_update && | |
| metadata?.autumn_invoice_mode !== "true" | |
| metadata?.autumn_billing_update && | |
| metadata?.autumn_invoice_mode !== "true" && | |
| latestInvoice?.status === "paid" |
| product_id: pro.id, | ||
| }); | ||
|
|
||
| console.log("Result:", result); |
Contributor
There was a problem hiding this comment.
P2: The new integration test has no behavioral assertions; it only logs the attach result, so regressions in billing behavior can pass undetected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/billing/attach/new-plan/attach-recurring-oneoff.test.ts, line 291:
<comment>The new integration test has no behavioral assertions; it only logs the attach result, so regressions in billing behavior can pass undetected.</comment>
<file context>
@@ -257,3 +257,36 @@ test.concurrent(`${chalk.yellowBright("recurring-oneoff 2: attach with only one-
+ product_id: pro.id,
+ });
+
+ console.log("Result:", result);
+});
</file context>
| import { createStripeCli } from "@/external/connect/createStripeCli"; | ||
| import type { AutumnContext } from "@/honoUtils/HonoEnv"; | ||
|
|
||
| const stripeDiscountsToInvoiceParams = ({ |
Contributor
There was a problem hiding this comment.
P2: This function duplicates the existing stripeDiscountsToParams in utils/discounts/stripeDiscountsToParams.ts. Consider reusing that function instead of duplicating the logic here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/billing/v2/providers/stripe/utils/invoices/createInvoiceForBilling.ts, line 19:
<comment>This function duplicates the existing `stripeDiscountsToParams` in `utils/discounts/stripeDiscountsToParams.ts`. Consider reusing that function instead of duplicating the logic here.</comment>
<file context>
@@ -12,19 +12,35 @@ import {
import { createStripeCli } from "@/external/connect/createStripeCli";
import type { AutumnContext } from "@/honoUtils/HonoEnv";
+const stripeDiscountsToInvoiceParams = ({
+ stripeDiscounts,
+}: {
</file context>
|
|
||
| await deleteCachedFullCustomer({ | ||
| ctx, | ||
| customerId: billingContext.fullCustomer.id ?? "", |
Contributor
There was a problem hiding this comment.
P2: Cache deletion fails to delete customer data when customerId falls back to an empty string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/billing/v2/execute/executeDeferredBillingPlan.ts, line 63:
<comment>Cache deletion fails to delete customer data when `customerId` falls back to an empty string.</comment>
<file context>
@@ -56,4 +57,10 @@ export const executeDeferredBillingPlan = async ({
+
+ await deleteCachedFullCustomer({
+ ctx,
+ customerId: billingContext.fullCustomer.id ?? "",
+ source: "executeInvoiceDeferredBillingPlan",
+ });
</file context>
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.
Summary by cubic
Improve Stripe billing accuracy and UX by linking upgrade invoices to subscriptions, applying discounts to allocated/overage invoices, and avoiding false past_due transitions. Shorter resume windows and better cache invalidation reduce stale state.
New Features
Bug Fixes
Written for commit 8144bc8. Summary will update on new commits.
Greptile Summary
This PR makes several improvements to Autumn's billing engine, primarily targeting correct status tracking for subscription upgrades, proper discount propagation to line items, support for consumable-only subscription invoicing, and customer cache invalidation after billing events.
Key changes:
[Bug fixes] Adds
handleFalsePositivePastDuelogic insyncCustomerProductStatusto prevent Stripe upgrade invoices (tagged withautumn_billing_updatemetadata) from incorrectly transitioning a customer topast_duestatus. However, the current check does not verify the invoice was actually paid — a genuine card failure on an Autumn-created upgrade invoice would still be suppressed. See inline comment.[Improvements] Attaches
autumn_billing_updateandautumn_invoice_modemetadata to all manually created Stripe invoices increateInvoiceForBilling, enabling downstream webhook handlers to distinguish Autumn-initiated invoices from Stripe-cycle invoices.[Improvements] Introduces
willStripeSubscriptionInvoiceEndOfCycleto detect when all subscription items are consumable prices. When true,addInvoiceItemsis omitted from the subscription create params and a separate manual invoice is created instead (shouldCreateManualStripeInvoice). This properly handles one-off consumable products.[Bug fixes] Adds Stripe discount application to the allocated invoice pipeline (
computeAllocatedInvoicePlan) and updatesapplyAmountOffDiscountToLineItems/applyPercentOffDiscountToLineItemsto use thediscountablecontext flag to decide whether to annotate descriptions with a discount tag.[Improvements] Adds
deleteCachedFullCustomercalls inexecuteDeferredBillingPlanand the two legacyhandleStripeInvoiceMetadatabranches to ensure customer state is evicted from cache after billing events complete.[API changes]
processInvoiceinInvoiceServicenow mapsinvoice.product_idsto theplan_idsfield in theApiInvoiceV1response (withproduct_idscommented out). This is a potential breaking change for API consumers relying onproduct_ids.[Bug fixes]
processConsumablePricesForSubscriptionDeletednow detects trial-end cancellations and skips arrear charges, preventing unexpected final invoices for customers whose free trials expired.Confidence Score: 2/5
syncCustomerProductStatusdoes not check whether the Autumn-created upgrade invoice was actually paid. If a card is declined on an upgrade invoice, the subscription moves topast_duebut the customer would be incorrectly shown asActive. The remaining changes (discount propagation, cache invalidation, consumable invoicing, metadata tagging) are well-structured and low-risk.server/src/external/stripe/webhookHandlers/handleStripeSubscriptionUpdated/tasks/syncCustomerProductStatus/syncCustomerProductStatus.ts— specifically thehandleFalsePositivePastDuefunction. Also reviewserver/src/internal/invoices/InvoiceService.tsfor theproduct_ids→plan_idsAPI field rename.Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Autumn participant Stripe participant Webhook as Stripe Webhook Client->>Autumn: Attach / upgrade plan Autumn->>Stripe: Create manual invoice<br/>(metadata: autumn_billing_update=true,<br/>autumn_invoice_mode=false) Stripe-->>Autumn: Invoice created alt Subscription has consumable-only items Autumn->>Stripe: Create subscription<br/>(addInvoiceItems: undefined) Autumn->>Stripe: Create separate manual invoice<br/>for one-off line items else Normal subscription Autumn->>Stripe: Create/update subscription<br/>(addInvoiceItems included) end Stripe->>Webhook: subscription.updated (status=past_due) Webhook->>Stripe: Fetch latest_invoice Stripe-->>Webhook: Invoice with metadata alt autumn_billing_update=true AND invoice_mode≠true Note over Webhook: False-positive suppressed →<br/>status stays Active else No autumn metadata or invoice_mode=true Note over Webhook: Genuine past_due →<br/>status set to PastDue end Webhook->>Autumn: Update CusProduct status Autumn->>Autumn: deleteCachedFullCustomerComments Outside Diff (2)
General comment
Leftover commented-out option
The
// skipSubscriptionCreateInvoice?: boolean;line inside the options type definition is dead code. It should either be removed or documented with aTODOexplaining why it was kept if it's planned for the future.Prompt To Fix With AI
General comment
product_idsrenamed toplan_idsin API response — verify consumersprocessInvoicenow mapsinvoice.product_idstoplan_idsinstead ofproduct_idsin theApiInvoiceV1response. The originalproduct_idsfield is commented out.If any external API consumers depend on
product_idsbeing present in the invoice response, this is a breaking change. Please ensure all SDK/client code that readsproduct_idsfrom invoice objects has been updated, or that this field rename is documented in a migration/changelog.Prompt To Fix With AI
Last reviewed commit: 8144bc8