Merged
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
+233
to
+235
| skipBillingChanges: | ||
| params.no_billing_changes ?? | ||
| params.processor_subscription_id !== undefined, |
Contributor
There was a problem hiding this comment.
skipBillingChanges operator inconsistency
The ?? operator only returns the right side when the left is null | undefined. If a caller explicitly passes no_billing_changes: false with a processor_subscription_id, this evaluates to false — Stripe operations will be attempted against an externally-owned subscription.
This is inconsistent with setupUpdateSubscriptionBillingContext.ts, which uses || and always skips billing when processor_subscription_id is present, regardless of no_billing_changes.
If the intent is "providing a processor_subscription_id always implies skipping billing changes", use ||:
Suggested change
| skipBillingChanges: | |
| params.no_billing_changes ?? | |
| params.processor_subscription_id !== undefined, | |
| skipBillingChanges: | |
| params.no_billing_changes === true || | |
| params.processor_subscription_id !== undefined, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/internal/billing/v2/actions/attach/setup/setupAttachBillingContext.ts
Line: 233-235
Comment:
**`skipBillingChanges` operator inconsistency**
The `??` operator only returns the right side when the left is `null | undefined`. If a caller explicitly passes `no_billing_changes: false` with a `processor_subscription_id`, this evaluates to `false` — Stripe operations will be attempted against an externally-owned subscription.
This is inconsistent with `setupUpdateSubscriptionBillingContext.ts`, which uses `||` and always skips billing when `processor_subscription_id` is present, regardless of `no_billing_changes`.
If the intent is "providing a `processor_subscription_id` always implies skipping billing changes", use `||`:
```suggestion
skipBillingChanges:
params.no_billing_changes === true ||
params.processor_subscription_id !== undefined,
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+4
to
+5
| test(`${chalk.yellowBright("processor_subscription_id: attach with existing stripe subscription anchors reset cycle")}`, async () => {}); | ||
|
|
Contributor
There was a problem hiding this comment.
Empty stub test files with no coverage
Both update-processor-no-billing-changes.test.ts and update-processor-status.test.ts are entirely empty stubs — all tests have empty bodies (async () => {}). These pass silently with zero coverage of the no_billing_changes and status parameter paths they claim to test.
Implement these tests before merging, or remove them and track in a follow-up ticket. The same issue exists in server/tests/integration/billing/update-subscription/params/update-processor-status.test.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/tests/integration/billing/update-subscription/params/update-processor-no-billing-changes.test.ts
Line: 4-5
Comment:
**Empty stub test files with no coverage**
Both `update-processor-no-billing-changes.test.ts` and `update-processor-status.test.ts` are entirely empty stubs — all tests have empty bodies (`async () => {}`). These pass silently with zero coverage of the `no_billing_changes` and `status` parameter paths they claim to test.
Implement these tests before merging, or remove them and track in a follow-up ticket. The same issue exists in `server/tests/integration/billing/update-subscription/params/update-processor-status.test.ts`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+127
to
+136
| await OrgService.update({ | ||
| db: db, | ||
| orgId: ctx.org.id, | ||
| updates: { | ||
| config: { | ||
| ...ctx.org.config, | ||
| skip_overage_submission: false, | ||
| }, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Org config not reset if test fails mid-way
skip_overage_submission is set to true at lines 67–76 and reset at lines 127–136, but only if the test reaches that point. If any assertion (e.g., expectCustomerInvoiceCorrect) throws, the org config remains polluted in the database, corrupting state for subsequent tests.
Wrap the test in a describe block with afterAll for guaranteed cleanup:
describe("skip overage submission", () => {
afterAll(async () => {
await OrgService.update({
db,
orgId: ctx.org.id,
updates: { config: { ...ctx.org.config, skip_overage_submission: false } },
});
});
test.concurrent(..., async () => { ... });
});Prompt To Fix With AI
This is a comment left during a code review.
Path: server/tests/integration/org-config/skip-overage-submission.test.ts
Line: 127-136
Comment:
**Org config not reset if test fails mid-way**
`skip_overage_submission` is set to `true` at lines 67–76 and reset at lines 127–136, but only if the test reaches that point. If any assertion (e.g., `expectCustomerInvoiceCorrect`) throws, the org config remains polluted in the database, corrupting state for subsequent tests.
Wrap the test in a `describe` block with `afterAll` for guaranteed cleanup:
```ts
describe("skip overage submission", () => {
afterAll(async () => {
await OrgService.update({
db,
orgId: ctx.org.id,
updates: { config: { ...ctx.org.config, skip_overage_submission: false } },
});
});
test.concurrent(..., async () => { ... });
});
```
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.
6 issues found across 49 files
Confidence score: 3/5
- There is moderate regression risk in billing flows:
server/src/internal/billing/v2/actions/attach/setup/setupAttachBillingContext.tsmay bypassshouldForceNewSubscription, allowingnew_billing_subscriptionto force Stripe subscription creation in cases that should be ignored. - Two medium-severity identifier-handling issues could cause invalid downstream behavior:
server/src/internal/entities/actions/batchCreateEntities/attachDefaultProductsToEntities.tscan proceed with a missing customer ID, andserver/src/cron/productCron/runProductCron.tscan send empty-string IDs to cache invalidation. - Test reliability is weaker than expected because
server/tests/integration/billing/update-subscription/params/update-processor-status.test.tsandserver/tests/integration/billing/update-subscription/params/update-processor-no-billing-changes.test.tscontain empty test bodies that always pass, reducing confidence in coverage. - Pay close attention to
server/src/internal/billing/v2/actions/attach/setup/setupAttachBillingContext.ts,server/src/internal/entities/actions/batchCreateEntities/attachDefaultProductsToEntities.ts, andserver/src/cron/productCron/runProductCron.ts- billing/subscription control and customer-ID validation need tightening to avoid incorrect operations.
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="server/src/internal/entities/actions/batchCreateEntities/attachDefaultProductsToEntities.ts">
<violation number="1" location="server/src/internal/entities/actions/batchCreateEntities/attachDefaultProductsToEntities.ts:54">
P2: Avoid defaulting to an empty customerId. If `fullCustomer.id` is missing, this will still execute billing logic with an invalid identifier. Guard and exit or throw when the ID is unavailable.</violation>
</file>
<file name="server/src/cron/productCron/runProductCron.ts">
<violation number="1" location="server/src/cron/productCron/runProductCron.ts:64">
P2: Avoid sending empty-string customer IDs to cache invalidation. Filter out rows with null customer IDs instead of coercing to "" to prevent invalid cache keys or errors.</violation>
</file>
<file name="server/tests/integration/billing/update-subscription/params/update-processor-status.test.ts">
<violation number="1" location="server/tests/integration/billing/update-subscription/params/update-processor-status.test.ts:4">
P2: These tests are empty and will always pass, so they don’t validate any behavior. Add assertions or remove the placeholders to avoid false test coverage.</violation>
</file>
<file name="server/tests/integration/billing/update-subscription/params/update-processor-no-billing-changes.test.ts">
<violation number="1" location="server/tests/integration/billing/update-subscription/params/update-processor-no-billing-changes.test.ts:4">
P3: These tests have empty bodies and will always pass without asserting any behavior, so they don’t validate the intended scenarios. Add assertions or test logic to verify the subscription behavior.</violation>
</file>
<file name="shared/api/billing/updateSubscription/updateSubscriptionV1Params.ts">
<violation number="1" location="shared/api/billing/updateSubscription/updateSubscriptionV1Params.ts:56">
P3: Validation message is outdated and no longer matches the accepted update fields.</violation>
</file>
<file name="server/src/internal/billing/v2/actions/attach/setup/setupAttachBillingContext.ts">
<violation number="1" location="server/src/internal/billing/v2/actions/attach/setup/setupAttachBillingContext.ts:119">
P2: Passing raw params into setupStripeBillingContext bypasses the shouldForceNewSubscription guard, so new_billing_subscription can force a new Stripe subscription even in transitions that should ignore it. This changes behavior for upgrades/downgrades and can skip fetching the existing subscription unexpectedly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| await executeAutumnBillingPlan({ | ||
| ctx, | ||
| autumnBillingPlan: { | ||
| customerId: fullCustomer.id ?? "", |
Contributor
There was a problem hiding this comment.
P2: Avoid defaulting to an empty customerId. If fullCustomer.id is missing, this will still execute billing logic with an invalid identifier. Guard and exit or throw when the ID is unavailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/entities/actions/batchCreateEntities/attachDefaultProductsToEntities.ts, line 54:
<comment>Avoid defaulting to an empty customerId. If `fullCustomer.id` is missing, this will still execute billing logic with an invalid identifier. Guard and exit or throw when the ID is unavailable.</comment>
<file context>
@@ -0,0 +1,59 @@
+ await executeAutumnBillingPlan({
+ ctx,
+ autumnBillingPlan: {
+ customerId: fullCustomer.id ?? "",
+ insertCustomerProducts,
+ },
</file context>
| customers: rows.map((row) => ({ | ||
| orgId: row.customer.org_id, | ||
| env: row.customer.env as AppEnv, | ||
| customerId: row.customer.id ?? "", |
Contributor
There was a problem hiding this comment.
P2: Avoid sending empty-string customer IDs to cache invalidation. Filter out rows with null customer IDs instead of coercing to "" to prevent invalid cache keys or errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/cron/productCron/runProductCron.ts, line 64:
<comment>Avoid sending empty-string customer IDs to cache invalidation. Filter out rows with null customer IDs instead of coercing to "" to prevent invalid cache keys or errors.</comment>
<file context>
@@ -1,84 +1,96 @@
+ customers: rows.map((row) => ({
+ orgId: row.customer.org_id,
+ env: row.customer.env as AppEnv,
+ customerId: row.customer.id ?? "",
+ })),
+ });
</file context>
| import { test } from "bun:test"; | ||
| import chalk from "chalk"; | ||
|
|
||
| test(`${chalk.yellowBright("processor_subscription_id: attach with existing stripe subscription anchors reset cycle")}`, async () => {}); |
Contributor
There was a problem hiding this comment.
P2: These tests are empty and will always pass, so they don’t validate any behavior. Add assertions or remove the placeholders to avoid false test coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/billing/update-subscription/params/update-processor-status.test.ts, line 4:
<comment>These tests are empty and will always pass, so they don’t validate any behavior. Add assertions or remove the placeholders to avoid false test coverage.</comment>
<file context>
@@ -0,0 +1,6 @@
+import { test } from "bun:test";
+import chalk from "chalk";
+
+test(`${chalk.yellowBright("processor_subscription_id: attach with existing stripe subscription anchors reset cycle")}`, async () => {});
+
+test(`${chalk.yellowBright("processor_subscription_id: upgrade with no_billing_changes preserves anchor and subscription")}`, async () => {});
</file context>
server/src/internal/billing/v2/actions/attach/setup/setupAttachBillingContext.ts
Show resolved
Hide resolved
| import { test } from "bun:test"; | ||
| import chalk from "chalk"; | ||
|
|
||
| test(`${chalk.yellowBright("processor_subscription_id: attach with existing stripe subscription anchors reset cycle")}`, async () => {}); |
Contributor
There was a problem hiding this comment.
P3: These tests have empty bodies and will always pass without asserting any behavior, so they don’t validate the intended scenarios. Add assertions or test logic to verify the subscription behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/billing/update-subscription/params/update-processor-no-billing-changes.test.ts, line 4:
<comment>These tests have empty bodies and will always pass without asserting any behavior, so they don’t validate the intended scenarios. Add assertions or test logic to verify the subscription behavior.</comment>
<file context>
@@ -0,0 +1,6 @@
+import { test } from "bun:test";
+import chalk from "chalk";
+
+test(`${chalk.yellowBright("processor_subscription_id: attach with existing stripe subscription anchors reset cycle")}`, async () => {});
+
+test(`${chalk.yellowBright("processor_subscription_id: upgrade with no_billing_changes preserves anchor and subscription")}`, async () => {});
</file context>
| ); | ||
| }).refine((data) => UPDATE_FIELDS.some((key) => data[key] !== undefined), { | ||
| message: | ||
| "At least one update parameter must be provided (feature_quantities, version, customize, or cancel_action)", |
Contributor
There was a problem hiding this comment.
P3: Validation message is outdated and no longer matches the accepted update fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At shared/api/billing/updateSubscription/updateSubscriptionV1Params.ts, line 56:
<comment>Validation message is outdated and no longer matches the accepted update fields.</comment>
<file context>
@@ -12,23 +13,48 @@ export const ExtUpdateSubscriptionV1ParamsSchema =
- );
+ }).refine((data) => UPDATE_FIELDS.some((key) => data[key] !== undefined), {
+ message:
+ "At least one update parameter must be provided (feature_quantities, version, customize, or cancel_action)",
+ });
</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
Introduces org-level default and billing flags, refactors the product cron to expire trials and activate free defaults at the right scope, and adds safe import paths to link existing Stripe subscriptions without changing them.
New Features
Bug Fixes
Written for commit d2f3907. Summary will update on new commits.
Greptile Summary
This PR introduces two new org-level configuration flags (
default_applies_to_entitiesandskip_overage_submission) and a set of internal billing parameters (processor_subscription_id,no_billing_changes,status) to support subscription import / migration workflows and entity-scoped default product behaviour.Key Changes:
default_applies_to_entitiesorg config flag: when enabled, free default products are attached to entities (not the top-level customer) on entity creation, trial expiry via cron, end-of-cycle cancellation, and Stripesubscription.deletedwebhook.skip_overage_submissionorg config flag: when enabled, consumable overage line items are not pushed to Stripe invoices oninvoice.createdandsubscription.deletedwebhook handlers.processor_subscription_id,no_billing_changes, andstatusas internal-only fields on both attach (V0/V1) and update-subscription (V0/V1) params, allowing callers to import an existing Stripe subscription without Autumn performing any Stripe operations.skipBillingChangesinBillingContext; when true, bothevaluateStripeBillingPlanandexecuteStripeBillingPlanare bypassed entirely.runProductCronwith bounded iteration (max 10 iterations, 1-minute guard, batch size 1 000) and adds entity-aware default product activation on trial expiry.customerProductEligibleForDefaultProduct, replacing scatteredcp().main().recurring().customerScoped()chains.internalEntityId/entityIdfromfullCus.entityintoproductToInsertParams, fixing entity context loss during attach parameter conversion.fetchStripeSubscriptionForBillingthat throws if the retrieved Stripe subscription belongs to a different Stripe customer.Remaining issues:
skipBillingChangesoperator in attach uses??instead of||, allowingno_billing_changes: falseto overrideprocessor_subscription_idpresence — inconsistent with the update context and could cause unintended Stripe operations.update-processor-no-billing-changes.test.ts,update-processor-status.test.ts) provide no coverage for the internal billing parameters they claim to test.skip-overage-submission.test.tsfile lacks properafterAllcleanup, risking database pollution if assertions fail mid-test.Confidence Score: 2/5
skipBillingChangesoperator in attach uses??instead of||, allowingno_billing_changes: falseto override the guard whenprocessor_subscription_idis provided — this could cause Stripe operations on externally-owned subscriptions; (2) Two stub test files have no test bodies and provide zero coverage of important parameters (no_billing_changes,status); (3) Theskip-overage-submission.test.tslacksafterAllcleanup, risking database state corruption if any assertion fails. The first issue is a logic bug that needs fixing. The second is missing test coverage that should be addressed. The third is a test hygiene issue.Last reviewed commit: 3d73d0e