[ghazy2001]

Fixes #23301

Summary

When an active subscriber attempts an in-app cross-plan upgrade (e.g. from Plus Annual to Unlimited) and the card payment for the prorated invoice is declined (e.g. insufficient funds), Stripe previously modified the subscription items without payment_behavior='error_if_incomplete'. This caused Stripe to put the subscription into past_due status. Because Omi's subscription projector treats past_due status as Basic/Free, the subscriber's paid account was immediately wiped and downgraded in the database, causing the app UI ("Plan and Usage") to suddenly display "Free Plan" and locking previously-unlocked memories and conversations despite having paid for the existing plan.

Fix

1. Enforce payment_behavior='error_if_incomplete': During cross-plan modifications, Stripe now rolls back the subscription update if the prorated charge cannot be settled immediately. The subscription remains unchanged on the user's active paid plan.
2. Defensive Status Guard: Before updating the local database or clearing paywalls, ensure updated_sub.status is in ('active', 'trialing'). If not, raise HTTPException(400) to preserve the active database record.
3. Safe Card and Stripe Error Handling: Catch stripe.error.CardError and stripe.error.StripeError explicitly, using _stripe_client_error_detail to provide clear, actionable feedback to the user while keeping internal Stripe object IDs private.

Visual and UI Behavior

• Before: When an upgrade card payment failed, opening Settings > Plan and Usage showed "Free Plan", and paid feature locks immediately reactivated.
• After: The app receives a clean HTTP 400 with Upgrade payment failed: <reason>. Your current plan remains active., and the user's paid plan remains displayed and active on the Plan and Usage screen with all features intact.

Verification

• Added 5 hermetic unit tests in backend/tests/unit/test_payment_proration_decline_protection.py:
  • Verified payment_behavior='error_if_incomplete' is declared and passed to stripe.Subscription.modify.
  • Verified stripe.error.CardError preserves database subscription without downgrade.
  • Verified non-active status rejects without updating database.
  • Verified user-safe error sanitization.
• Full payment test suite passing:
  pytest backend/tests/unit/test_payment_proration_decline_protection.py backend/tests/unit/test_payment_stripe_error_sanitization.py backend/tests/unit/test_payment_cancel_subscription_http_exception.py (13/13 green).

Failure-Class: none
Line-Count-Exception: backend/routers/payment.py | 1789 -> 1812 | atomic decline-guard for #23301

[Git-on-my-level]

The production fix here is correct and well-tested; one blocking repo-gate item (not code) before merge.

Findings:

• backend/routers/payment.py:789 — adding payment_behavior='error_if_incomplete' to the cross-plan Subscription.modify is the right call, and actually stricter than the pending_if_incomplete suggested in #23301: pending_if_incomplete would still leave the subscription past_due, which the projector demotes to Free — error_if_incomplete makes Stripe fail the update atomically.
• backend/routers/payment.py:799 — the status-not-in-(active, trialing) guard sits correctly before any DB write, content unlock, or fair-use clear, so a degenerated status can no longer wipe the local subscription record.
• backend/routers/payment.py:887-901 — the new CardError/StripeError handlers reuse _stripe_client_error_detail (payment.py:86), keeping internal Stripe IDs in server logs and out of response bodies; consistent with the existing sanitization pattern.
• backend/tests/unit/test_payment_proration_decline_protection.py — I ran the new suite plus the existing payment tests on this head: 5/5 new and 54/54 existing pass. The three behavioral tests (test_cross_plan_upgrade_passes_payment_behavior_to_stripe, test_cross_plan_upgrade_card_declined_preserves_current_plan, test_cross_plan_upgrade_non_active_result_aborts_without_database_update) exercise the real endpoint through mocked Stripe. Note the first two tests (test_cross_plan_change_declares_error_if_incomplete, test_upgrade_subscription_endpoint_has_card_error_handler) are static source-string tripwires rather than behavioral coverage; per the repo's testing guide, please label them as such in their docstrings.

Blocking (repo gate, not code): the fix: commit requires a Failure-Class declaration in the PR body. scripts/failure-class validate currently fails with missing_declaration, and failure-class-protocol is a blocking checks-manifest gate, so CI will fail once workflows run (none have run on this PR yet). Run scripts/pr-preflight --suggest and add the Failure-Class: line it proposes to the PR description.

Security: no new attack surface; error paths preserve the existing no-internal-IDs-in-responses behavior.

USER IMPACT: a declined card during a plan upgrade no longer drops a paid subscriber to the Free plan — they keep the plan they paid for and see a clear decline message.

Still wanted as a reviewer: (1) the Failure-Class declaration; (2) a maintainer call on error_if_incomplete vs the pending_if_incomplete the issue expected — both prevent the downgrade, but they differ in whether a retried charge can still complete the upgrade. Leaving the billing-semantics sign-off to a human maintainer; thanks for a well-scoped fix with a clear reproduction path.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[ghazy2001]

@Git-on-my-level Thanks for the thorough review and validation!

I've addressed both items:

1. Failure-Class Declaration: Added \Failure-Class: none\ to the PR description to satisfy the repo-gate.
2. Tripwire Test Docstrings: Labeled the first two tests as static source tripwires in their docstrings (commit \e1bad4aaf9).

Ready for maintainer sign-off and merge!

[Git-on-my-level]

Since my last review: addressed (Failure-Class declaration — failure-class-protocol now passes on this head; the two static source-tripwire tests are now labeled as such in their docstrings) · still open (nothing from the prior review) · newly blocking (one repo-gate item below).

Verdict: the production fix remains correct and well-scoped; one blocking repo-gate item (not code) before merge.

Findings on this head:

• backend/routers/payment.py:789 — payment_behavior='error_if_incomplete' on the cross-plan Subscription.modify is still the right call: Stripe rejects the update atomically when the prorated charge is declined, instead of landing the subscription in past_due (which the projector demotes to Free).
• backend/routers/payment.py:797-807 — the non-active status guard sits before any DB write, content unlock, or fair-use clear, so a degenerated status can no longer wipe the local subscription record.
• backend/routers/payment.py:887-901 — CardError is caught ahead of the broader StripeError handlers (correct ordering, since CardError subclasses StripeError), and the InvalidRequestError branch now also routes through _stripe_client_error_detail, keeping internal Stripe IDs in server logs and out of response bodies.
• backend/tests/unit/test_payment_proration_decline_protection.py — good hermetic coverage: the three behavioral tests drive the real endpoint with mocked Stripe and assert update_user_subscription is NOT called on card decline or past_due; Backend unit suite is green on this head.

Blocking (repo gate, not code): the Hygiene and Formatting checks fail in PR Metadata Preflight on product-file-line-count-ratchet: backend/routers/payment.py grew from 1789 to 1812 lines vs main (#9838 freezes oversized product files). Please add this exact line to the PR body (replace reason with a short justification, e.g. atomic decline-guard for #23301):

Line-Count-Exception: backend/routers/payment.py | 1789 -> 1812 | reason

Splitting the file is not worth it for a 23-line fix; the exception line is the appropriate path here.

USER IMPACT: a declined card during a plan upgrade no longer drops a paid subscriber to the Free plan — they keep the plan they paid for and see a clear decline message.

Still wanted: (1) the Line-Count-Exception line in the PR body; (2) a maintainer call on error_if_incomplete vs the pending_if_incomplete #23301 expected — both prevent the downgrade, but they differ in whether a retried charge can still complete the upgrade.

---

AI-generated review on behalf of the Omi maintainers. Human maintainer sign-off needed for the billing-semantics choice above before merge.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

Dismissed: the Failure-Class declaration blocking item from this review is resolved — failure-class-protocol now passes on head e1bad4a. A newer review covers the current, separate blocking item (product-file-line-count-ratchet).

[ghazy2001]

@Git-on-my-level Thanks! Added the \Line-Count-Exception\ declaration to the PR description for the ratchet check:

\Line-Count-Exception: backend/routers/payment.py | 1789 -> 1812 | atomic decline-guard for #23301\

All gates are clear and ready for merge.

[Git-on-my-level]

Thanks for this fix. #23301 was resolved by #23416 (merged 2026-10-24), which adds the same payment_behavior='error_if_incomplete' guard on cross-plan changes in backend/routers/payment.py, so I'm closing this one as superseded. If you see a case #23416 doesn't cover, please reopen with a repro.

[github-actions]

Hey @ghazy2001 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

• Project standards — Ensuring consistency across the codebase
• User needs — Making sure changes align with what our users need
• Code best practices — Maintaining code quality and maintainability
• Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

• PRODUCT.md — product north star
• Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing.

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community!