fix: keep catalog push alive when deletion checks fail - #2314
fix: keep catalog push alive when deletion checks fail#2314charlietlamb wants to merge 5 commits into
Conversation
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch { | ||
| return { | ||
| id: planId, | ||
| canDelete: false, | ||
| customerCount: response.totalCount, | ||
| firstCustomerName: response.customerName, | ||
| customerCount: 0, | ||
| deletionCheckFailed: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
deletionCheckFailed flag is never read — fail-closed intent is not enforced
When getPlanDeletionInfo throws, the catch block returns canDelete: false, customerCount: 0, deletionCheckFailed: true. Because customerCount === 0, createPlanDeletePrompt routes to createPlanDeleteNoCustomersPrompt, which presents "Delete permanently" as the default option. Nothing in usePush.ts (or anywhere else) reads deletionCheckFailed to override this behaviour, so if the user accepts the default the push will attempt to delete a plan whose customer status is genuinely unknown. The same pattern appears in checkFeatureDeleteInfo: reason: "deletion_check_failed" falls through createFeatureDeletePrompt to createFeatureDeleteNoDepsPrompt, again defaulting to "Delete permanently" as though the feature has no dependencies. Both cases contradict the PR's stated "fail closed and defer deletion" goal. At minimum the prompt should default to "Skip (keep as is)" when deletionCheckFailed is true; alternatively the failed-check entries could be excluded from plansToDelete / featuresToDelete entirely and silently deferred.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/atmn/src/commands/push/push.ts
Line: 230-237
Comment:
**`deletionCheckFailed` flag is never read — fail-closed intent is not enforced**
When `getPlanDeletionInfo` throws, the catch block returns `canDelete: false, customerCount: 0, deletionCheckFailed: true`. Because `customerCount === 0`, `createPlanDeletePrompt` routes to `createPlanDeleteNoCustomersPrompt`, which presents **"Delete permanently"** as the default option. Nothing in `usePush.ts` (or anywhere else) reads `deletionCheckFailed` to override this behaviour, so if the user accepts the default the push will attempt to delete a plan whose customer status is genuinely unknown. The same pattern appears in `checkFeatureDeleteInfo`: `reason: "deletion_check_failed"` falls through `createFeatureDeletePrompt` to `createFeatureDeleteNoDepsPrompt`, again defaulting to "Delete permanently" as though the feature has no dependencies. Both cases contradict the PR's stated "fail closed and defer deletion" goal. At minimum the prompt should default to "Skip (keep as is)" when `deletionCheckFailed` is true; alternatively the failed-check entries could be excluded from `plansToDelete` / `featuresToDelete` entirely and silently deferred.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| if (response && response.totalCount > 0) { | ||
| if (response && response.totalCount > 0) { |
There was a problem hiding this comment.
if statement — it is left over from the refactor and the surrounding code is at a single-tab indent level.
| } | |
| if (response && response.totalCount > 0) { | |
| if (response && response.totalCount > 0) { | |
| } | |
| if (response && response.totalCount > 0) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/atmn/src/commands/push/push.ts
Line: 192-194
Comment:
Extra indentation on the `if` statement — it is left over from the refactor and the surrounding code is at a single-tab indent level.
```suggestion
}
if (response && response.totalCount > 0) {
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
Confidence score: 2/5
- In
packages/atmn/src/commands/push/push.ts, the license transform path appears to dropcustomize/prepaidOnlyand optional customize-license fields, which can rewrite omittedincludedsemantics and send materially different license payloads than intended. Merging as-is risks silently changing plan configuration on push — preserve optional fields in the API↔SDK mappings and re-run normalization/compare against full license data before merging. - In
server/src/internal/catalog/actions/updateCatalog/updateCatalog.ts, the recovery path passesvariantUpdateswithoutpropagateToVariants, so selected variants may not be updated whenupdate_variant_idsis present and the requested version is missing. Merging this path as-is can leave catalog variants out of sync after recovery — includepropagateToVariants(or equivalent propagation behavior) in that call before merging.
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/catalog/actions/updateCatalog/updateCatalog.ts">
<violation number="1" location="server/src/internal/catalog/actions/updateCatalog/updateCatalog.ts:183">
P2: Selected variants are not updated when this recovery path is taken: the call passes `variantUpdates` but omits `propagateToVariants`. When a catalog plan has `update_variant_ids` and its requested version is missing, the base product is recovered but the requested variant propagation is silently skipped.</violation>
</file>
<file name="packages/atmn/src/commands/push/push.ts">
<violation number="1" location="packages/atmn/src/commands/push/push.ts:590">
P2: License comparison does not actually observe the new `prepaidOnly` or `customize` values because the API-to-SDK transform drops both fields before `normalizePlanForCompare` runs. Updating the license model and transform alongside this comparison would prevent these lines from being dead and ensure plan changes are detected.</violation>
<violation number="2" location="packages/atmn/src/commands/push/push.ts:696">
P1: Variant license customization loses its optional fields and changes the meaning of omitted `included` values in every payload using this path. Mapping customize-license entries directly while preserving optional `included`, `prepaidOnly`, and nested `customize` fields would avoid sending `0` or dropping the requested customization.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| : {}), | ||
| ...(customize.upsertLicenses !== undefined | ||
| ? { | ||
| upsert_licenses: transformPlanToApi({ |
There was a problem hiding this comment.
P1: Variant license customization loses its optional fields and changes the meaning of omitted included values in every payload using this path. Mapping customize-license entries directly while preserving optional included, prepaidOnly, and nested customize fields would avoid sending 0 or dropping the requested customization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/atmn/src/commands/push/push.ts, line 696:
<comment>Variant license customization loses its optional fields and changes the meaning of omitted `included` values in every payload using this path. Mapping customize-license entries directly while preserving optional `included`, `prepaidOnly`, and nested `customize` fields would avoid sending `0` or dropping the requested customization.</comment>
<file context>
@@ -668,6 +691,15 @@ function toApiCustomizePlan(customize: CustomizePlan): Record<string, unknown> {
: {}),
+ ...(customize.upsertLicenses !== undefined
+ ? {
+ upsert_licenses: transformPlanToApi({
+ id: "license-customize",
+ name: "License customize",
</file context>
| query: { force_version: true }, | ||
| updates, | ||
| initialFullProduct: latest, | ||
| variantUpdates, |
There was a problem hiding this comment.
P2: Selected variants are not updated when this recovery path is taken: the call passes variantUpdates but omits propagateToVariants. When a catalog plan has update_variant_ids and its requested version is missing, the base product is recovered but the requested variant propagation is silently skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/catalog/actions/updateCatalog/updateCatalog.ts, line 183:
<comment>Selected variants are not updated when this recovery path is taken: the call passes `variantUpdates` but omits `propagateToVariants`. When a catalog plan has `update_variant_ids` and its requested version is missing, the base product is recovered but the requested variant propagation is silently skipped.</comment>
<file context>
@@ -158,6 +158,32 @@ const upsertPlans = async ({
+ query: { force_version: true },
+ updates,
+ initialFullProduct: latest,
+ variantUpdates,
+ });
+ continue;
</file context>
| variantUpdates, | |
| variantUpdates, | |
| propagateToVariants: update_variant_ids ?? [], |
| version: | ||
| license.version ?? licenseVersionFallbacks?.get(license.licensePlanId), | ||
| included: license.included ?? 0, | ||
| prepaidOnly: license.prepaidOnly ?? true, |
There was a problem hiding this comment.
P2: License comparison does not actually observe the new prepaidOnly or customize values because the API-to-SDK transform drops both fields before normalizePlanForCompare runs. Updating the license model and transform alongside this comparison would prevent these lines from being dead and ensure plan changes are detected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/atmn/src/commands/push/push.ts, line 590:
<comment>License comparison does not actually observe the new `prepaidOnly` or `customize` values because the API-to-SDK transform drops both fields before `normalizePlanForCompare` runs. Updating the license model and transform alongside this comparison would prevent these lines from being dead and ensure plan changes are detected.</comment>
<file context>
@@ -572,6 +587,8 @@ function normalizePlanForCompare(
version:
license.version ?? licenseVersionFallbacks?.get(license.licensePlanId),
included: license.included ?? 0,
+ prepaidOnly: license.prepaidOnly ?? true,
+ customize: license.customize,
}));
</file context>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
1 participant
Prevents catalog preview/push from crashing on partially-deleted products with missing entitlements.
bun run buildValidation: focused atmn push tests pass; full Autumn typecheck is currently blocked by pre-existing dependency/type errors.
Summary by cubic
Keeps catalog preview/update/push running when deletion checks fail, and adds explicit plan versioning across catalog and billing sync. Stripe price mapping now pins the exact plan version, including schedule phases and renamed plans.
Bug Fixes
deletion_check_failedflags.New Features
new_plan_id).variant.version,additional_currencies,customize.upsert_licenses, and default licenseprepaidOnly.Written for commit 222be1c. Summary will update on new commits.
Greptile Summary
This PR prevents
catalog.previewandcatalog.pushfrom crashing when encountering partially-deleted products with missing entitlements, and adds first-class support for pushing explicit plan versions in a single batch.product.entitlementswith?? []inpreviewUpdateCatalog.tsto prevent crashes on null entitlements; wrapsgetFeatureDeletionInfoandgetPlanDeletionInfoAPI calls in try/catch so failures defer deletion rather than aborting the whole push.validateCatalogPlanVersionTargetsto enforce sequential version numbering; extends the dependency sort to order earlier versions before later ones; adds a newupsertPlansbranch to create versioned plans against an existing base; propagatesversionthrough billing sync params so Stripe price matching pins the correct plan version.normalizePlanForComparenow includesprepaidOnlyandcustomizeon licenses;toApiCustomizePlanforwardsadditional_currencies;toCatalogVariantParamsforwardsversion.Confidence Score: 3/5
Server-side changes are safe to merge; the client-side deletion guard in push.ts behaves differently from what the PR description promises.
The server-side catalog and billing changes are well-structured and covered by new integration and unit tests. The push.ts fix is incomplete: when the deletion-check API call fails, the code returns canDelete: false and sets deletionCheckFailed: true, but that flag is never read anywhere in the prompt generation or execution pipeline. Both the feature path and the plan path present 'Delete permanently' as the default action when a check fails, which is the opposite of conservative.
packages/atmn/src/commands/push/push.ts and packages/atmn/src/commands/push/prompts.ts — the deletionCheckFailed/deletion_check_failed path needs to either auto-skip or route to a prompt that defaults to Skip.
Important Files Changed
deletionCheckFailedguard is stored but never read, causing both feature and plan prompt routing to default to "Delete permanently" when a check fails.deletionCheckFailed?: booleantoPlanDeleteInfoand a new"deletion_check_failed"reason toFeatureDeleteInfo; both additions are correct but the plan field is currently dead code.validateCatalogPlanVersionTargetsthat enforces sequential version numbering for new plan versions; logic is correct and well-tested.validateCatalogPlanVersionTargetsbefore virtual product construction and respects explicit version numbers on new plans; straightforward and correct.force_version: trueand delegates toupdateProduct, which is consistent with the existing versioning mechanism.product.entitlementswith?? []to prevent crashes on partially-deleted products; targeted one-line fix that addresses the stated bug.versionto the sync plan params so that billing sync pins the exact matched product version; tested by the new integration test.validateCatalogPlanVersionTargetspath.["child@1", "child@2", "parent@0"]after the previous-version traversal change, and adds a unit test for gap-version rejection.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: keep catalog push alive when deleti..." | Re-trigger Greptile