Fix catalog migration review findings for #1155 - #1203
Merged
think-back merged 9 commits intoSep 17, 2026
Merged
think-back merged 9 commits into
think-back merged 9 commits into
Conversation
…pe test mode The snapshot validator rejected every live-mode Stripe Price while the production sandbox gate requires live-mode facts, so production preview and scheduling could never succeed. Mode consistency stays enforced by ValidateCatalogMigrationStripeSandbox. Also checks in the review-fix implementation plan.
… paid invoices after discount release Renewal validation now proves ownership (subscription, item, customer, binding, contract), price id, currency and quantity only. Amount equality against local prices was removed everywhere: tax, customer credit balance, coupons and rounding legitimately change what Stripe collects and turned genuinely paid invoices into permanent, non-retryable failures. - A discount reservation that was already released no longer blocks the paid renewal grant; only the ledger commit is skipped. - Version 1 discount snapshots (reserved before typed renewal ownership existed) stay honored on bindings that gained a typed plan snapshot. - Re-preparing an already adjusted draft invoice no longer fails on the recomputed subtotal. - Discount fingerprints fall back to plan identity for renewals without a frozen snapshot (reached scheduled downgrades).
…nges A typed plan snapshot frozen for one plan can be left on a binding that later moves to another plan through an upgrade or downgrade. Treat a snapshot whose plan id no longer matches the binding as absent instead of failing every future renewal permanently, and clear it when a reached scheduled downgrade moves the binding to another catalog plan.
…ary and keep the wallet sweeper alive Renewal-boundary semantics are now explicit: the feature flag and contract allowlist gate admin preview/apply/cancel only. A scheduled cutover is a committed fact and still applies after the flag is closed. Only runtime drift (batch prepared on another service, environment or Stripe mode) blocks it, and that error is retryable rather than a permanent webhook ack. - Wallet loader treats every non-scheduled catalog intent as "no cutover" instead of erroring forever once an intent is applied. - Stripe loader applies compensation_required/needs_attention intents only when the paid invoice already bills the target price; otherwise the invoice is an ordinary legacy renewal. - RunWalletSubscriptionRenewalOnce logs and skips a failing contract instead of aborting the batch (which also skipped term advance, expiry and reset). - A contract paused for insufficient balance may have its entitlement expired by the expiry task; that state is accepted so the top-up retry and the cutover can still happen. - Ordinary typed-snapshot renewals no longer require the current catalog row to match the frozen snapshot.
…per-contract apply errors - Cancel persists status=cancelled on the batch. Get/refreshBatchSummary keep it sticky, and Apply refuses a cancelled batch, so a replayed apply can no longer schedule a contract whose first prepare failed before an intent row existed. - The summary counts syncing intents as in-flight (batch stays applying) instead of as failures. - processCatalogMigrationPreview logs each per-contract apply/resume error with batch and contract ids instead of discarding it.
…erflow Go's AddDate normalizes Jan 31 + 1 month to Mar 3. The schedule wrote a longer-than-monthly target phase for subscribers anchored on the 29th to 31st, and the ownership re-check (same expression) then never matched what Stripe stored, parking the intent in needs_attention. Clamp to the last day of the following month, preserving the time of day.
…g and respect reservation expiry - supersedeCatalogMigrationForUserAction takes the client's ExpectedChangeVersion and enforces it inside the locked transaction, before the Stripe schedule release and the intent supersede. A stale cancel now fails the precondition with no side effect instead of releasing the schedule and then overwriting the precondition to pass. - Wallet contracts (no provider binding) supersede a scheduled local cutover on user cancel/plan change, so the next renewal tick cannot silently undo the user's action. - The user-action attention marker is status-guarded: an intent that a concurrent paid renewal applied (or a cancel superseded) is never relabelled needs_attention and its contract is not frozen. - Busy checks (preview and user action) use the shared active-reservation predicate: consumed tombstones and expired leases no longer read as provider_lifecycle_busy. Database time is read on the transaction handle to avoid a second connection inside a locked transaction. - containsString is replaced by common.StringsContains.
… rules - Drop gorm type:longtext from the four snapshot columns; the dialector maps an unsized string to longtext on MySQL and text on PostgreSQL, so AutoMigrate no longer emits a MySQL-only type on PostgreSQL (Rule 2). - Route all JSON encoding through common.* instead of encoding/json in business code (Rule 1); the digest inputs are byte-identical. - Replace parseCatalogMigrationBool with common.GetEnvOrDefaultBool and delete the unused isCatalogMigrationTerminalProviderStatus. - Carry the applied cutover's batch id out of the paid-invoice transaction on the result instead of an unindexed intent lookup after every paid invoice.
The env export loop drops empty values and --update-env-vars is merge-only, so an allowlist set by hand on the running service could never be cleared by a deploy. Write the (possibly empty) allowlist explicitly in both production jobs.
think-back
merged commit Sep 17, 2026
e19548a
into
feat/staging-legacy-renewal-catalog-migration
1 check passed
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
Review fixes for #1155 (legacy → new catalog cutover at paid renewal), stacked on
feat/staging-legacy-renewal-catalog-migration. Every finding of the 2026-09-14 review is addressed with a failing test first, one commit per finding group. External API and the two-phase design are unchanged.Plan and finding map:
docs/superpowers/plans/2026-09-14-pr1155-catalog-migration-review-fixes.md.Blocking defects fixed
ValidateCatalogMigrationStripeSandboxfacts.Amountequality on renewal (tax / credit balance / coupons / rounding → permanent non-retryable failure of genuinely paid invoices); released reservation blocked the grant; v1 discount snapshots rejected; re-prepare failed on recomputed subtotalstatus=cancelledpersisted and sticky;Applyrefuses a cancelled batchAddDate(0,1,0)month overflow (Jan 31 → Mar 3) → ownership re-check never matchedcatalogMigrationNextMonthlyPeriodEndclamps to month end, time of day preservedExpectedChangeVersion, then overwrote it; attention marker relabelled applied intents; busy checks ignored expiry/tombstones; wallet contracts never superseded catalog intentsSubscriptionProviderLifecycleReservationIsActive; wallet supersede pathtype:longtext(Rule 2),encoding/jsonin business code (Rule 1), duplicated helpers, dead func, unindexed post-tx lookup on every paid invoicecommon.*JSON,common.GetEnvOrDefaultBool/common.StringsContains, batch id carried out of the tx on the result--update-env-vars(merge-only) → a hand-set production allowlist could never be cleared by a deployDeliberately kept
context.Background()for the user-action supersede (a multi-step provider mutation should not be cancelled by a client disconnect).Verification
go vet ./model ./controller ./router ./serviceclean.go test ./service/on every touched suite (catalog migration, plan snapshot, invoice, discount, wallet renewal, renewal lifecycle, price authority) passes;./router/and./controller/ -run CatalogMigrationpass; model catalog-migration/lifecycle suites pass.39b32eed6:TestChangeSubscriptionPlanGateDisabledPreservesLegacyBehavior,TestStripeUpgradeReplay*(2),TestSQLiteQuotaLifecycleMigrationWidensLegacyCycleAndSourceColumns, and the 4 controller subscription tests already noted in Renew legacy subscriptions into the new catalog at paid renewal #1155.Deployment
Same footprint as #1155: router and console both serve renewal paths (paid-invoice webhook, wallet sweeper, AutoMigrate); staging via the
stagingbranch; both GitHub Actions workflows. No frontend, Terraform or Cloudflare changes.