egullickson/motovaultpro
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: Clean up subscription admin override and Stripe integration #205

New Issue
Closed
opened 2026-02-16 14:41:37 +00:00 by egullickson · 6 comments
egullickson commented 2026-02-16 14:41:37 +00:00
Owner
Copy Link
Copy Source

Summary

Refactor the subscription system to cleanly separate admin-managed tier overrides from Stripe-managed billing. The current implementation uses an admin_override_ placeholder in the stripe_customer_id column, which violates column semantics, has caused bugs (#58), and was implemented without approval. This refactor also hardens the dual-table tier sync to prevent future drift.

Related Issues

  • Original Stripe implementation: #55
  • Tier gating system: #8
  • Tier sync bug (fixed): #58

Background

Problem 1: admin_override_ Placeholder Pattern

When an admin overrides a user's tier, the system generates a fake Stripe customer ID:

// subscriptions.repository.ts:583
const placeholderCustomerId = `admin_override_${userId}_${Date.now()}`;

This is stored in subscriptions.stripe_customer_id (VARCHAR(255) UNIQUE NOT NULL). Every code path that touches this column must check for the admin_override_ prefix:

  • resolveStripeCustomerId() detects it during upgrades and creates a real Stripe customer
  • getInvoices() returns [] when it detects the prefix
  • Payment method endpoints must skip Stripe calls for these records

Root cause: The stripe_customer_id column has a NOT NULL constraint, forcing a placeholder value when Stripe is not involved.

Problem 2: Non-Blocking Tier Sync

The syncTierToUserProfile() method (service lines 804-816) swallows errors:

// Don't throw - we don't want to fail the subscription operation if sync fails

While the admin override uses a transaction for atomic sync, non-admin paths (webhooks, upgrades, downgrades) use this non-blocking sync. If user_profiles.subscription_tier fails to update, the auth context reads stale tier data.

Price IDs vs Product IDs (Non-Issue)

The system uses price_ IDs (not product_ IDs) when creating Stripe subscriptions. Per Stripe's official documentation, this is correct behavior. Stripe Products define what you sell; Prices define how much and how often. subscriptions.create requires a price parameter. The getPriceId() method mapping tier+billingCycle to environment variables is the standard pattern.

Proposed Changes

1. Make stripe_customer_id NULLABLE

Migration: ALTER subscriptions.stripe_customer_id to drop NOT NULL constraint:

ALTER TABLE subscriptions ALTER COLUMN stripe_customer_id DROP NOT NULL;
  • Clean up existing admin_override_* values to NULL
  • Remove the resolveStripeCustomerId() lazy resolution method
  • When user upgrades from admin-set tier, create Stripe customer directly in the upgrade flow
  • Webhook handlers already use findByStripeCustomerId() - NULL records will never match (correct behavior)

2. Make syncTierToUserProfile() Blocking

Change from non-blocking (error swallowed) to blocking (throw on failure):

private async syncTierToUserProfile(userId: string, tier: SubscriptionTier): Promise<void> {
  await this.userProfileRepository.updateSubscriptionTier(userId, tier);
  logger.info('Subscription tier synced to user profile', { userId, tier });
  // Errors now propagate to caller
}

This is safe because:

  • Single-row UPDATE on indexed column takes <1ms
  • No Stripe webhook timeout risk
  • Prevents tier drift in ALL code paths (upgrades, downgrades, webhooks)

3. Clean Admin Override Flow

Remove createForAdminOverride() placeholder logic. The admin flow becomes:

  1. Check if subscription record exists
  2. If not, create with stripe_customer_id = NULL, stripe_subscription_id = NULL
  3. Set tier and status atomically in transaction (both tables)
  4. When user later pays via Stripe, upgrade flow creates the Stripe customer

Files Affected

Backend:

  • backend/src/features/subscriptions/data/subscriptions.repository.ts - Remove createForAdminOverride(), update queries for nullable stripe_customer_id
  • backend/src/features/subscriptions/domain/subscriptions.service.ts - Remove resolveStripeCustomerId(), update adminOverrideTier(), make syncTierToUserProfile() blocking
  • backend/src/features/subscriptions/api/subscriptions.controller.ts - Update getInvoices/payment-method to check for null instead of prefix
  • backend/src/features/subscriptions/migrations/ - New migration for nullable stripe_customer_id

Frontend (minor):

  • frontend/src/features/subscription/pages/SubscriptionPage.tsx - Conditional rendering for invoices/payment when no Stripe customer

Acceptance Criteria

  • stripe_customer_id column is NULLABLE (migration applied)
  • All existing admin_override_* values migrated to NULL
  • resolveStripeCustomerId() method removed
  • createForAdminOverride() creates records with NULL stripe_customer_id
  • Admin can set any tier (free/pro/enterprise) without Stripe interaction
  • User with admin-set tier can later upgrade via Stripe payment (new Stripe customer created in upgrade flow)
  • User with admin-set tier that is downgraded can still pay for upgrade via Stripe
  • syncTierToUserProfile() throws on failure (blocking)
  • Webhook handlers continue to function (NULL stripe_customer_id records are never matched)
  • Invoices endpoint returns [] for users without Stripe customer
  • Payment method endpoint gracefully handles NULL stripe_customer_id
  • All linters pass
  • All tests pass
  • Mobile + desktop responsive validation

Test Plan

  1. Admin Override Flow:

    • Admin sets user from Free to Pro - verify both tables updated, no placeholder in stripe_customer_id
    • Admin sets user from Pro to Free - verify downgrade, vehicle selection prompt if needed
    • Admin sets user from Free to Enterprise - verify upgrade

  2. Stripe Upgrade After Admin Override:

    • Admin sets user to Pro (no Stripe)
    • Admin downgrades to Free
    • User pays for Pro via Stripe - verify new Stripe customer created, subscription active
    • Verify webhooks work for this user going forward

  3. Blocking Sync:

    • Verify tier changes fail atomically if user_profiles update fails
    • Verify webhook-triggered tier changes sync correctly

  4. Regression:

    • Normal Stripe upgrade flow (no admin involvement) still works
    • Normal Stripe downgrade flow still works
    • Webhook processing still works
    • Grace period job still processes correctly

Decision Critic Results

Decisions stress-tested via Decision Critic skill. Key findings:

  • Eliminating admin_override_ entirely: VERIFIED - NULLABLE stripe_customer_id correctly isolates admin subscriptions from Stripe webhooks
  • Blocking sync: VERIFIED - Safe, <1ms operation, prevents drift in all paths
  • Manual tier change only: VERIFIED - Seamless upgrade path preserved
  • UUID migration: SEPARATED into independent issue (not bundled here to reduce risk)
## Summary Refactor the subscription system to cleanly separate admin-managed tier overrides from Stripe-managed billing. The current implementation uses an `admin_override_` placeholder in the `stripe_customer_id` column, which violates column semantics, has caused bugs (#58), and was implemented without approval. This refactor also hardens the dual-table tier sync to prevent future drift. ## Related Issues - Original Stripe implementation: #55 - Tier gating system: #8 - Tier sync bug (fixed): #58 ## Background ### Problem 1: `admin_override_` Placeholder Pattern When an admin overrides a user's tier, the system generates a fake Stripe customer ID: ```typescript // subscriptions.repository.ts:583 const placeholderCustomerId = `admin_override_${userId}_${Date.now()}`; ``` This is stored in `subscriptions.stripe_customer_id` (VARCHAR(255) UNIQUE NOT NULL). Every code path that touches this column must check for the `admin_override_` prefix: - `resolveStripeCustomerId()` detects it during upgrades and creates a real Stripe customer - `getInvoices()` returns `[]` when it detects the prefix - Payment method endpoints must skip Stripe calls for these records **Root cause**: The `stripe_customer_id` column has a `NOT NULL` constraint, forcing a placeholder value when Stripe is not involved. ### Problem 2: Non-Blocking Tier Sync The `syncTierToUserProfile()` method (service lines 804-816) swallows errors: ```typescript // Don't throw - we don't want to fail the subscription operation if sync fails ``` While the admin override uses a transaction for atomic sync, non-admin paths (webhooks, upgrades, downgrades) use this non-blocking sync. If `user_profiles.subscription_tier` fails to update, the auth context reads stale tier data. ### Price IDs vs Product IDs (Non-Issue) The system uses `price_` IDs (not `product_` IDs) when creating Stripe subscriptions. Per Stripe's official documentation, this is **correct behavior**. Stripe Products define what you sell; Prices define how much and how often. `subscriptions.create` requires a `price` parameter. The `getPriceId()` method mapping tier+billingCycle to environment variables is the standard pattern. ## Proposed Changes ### 1. Make `stripe_customer_id` NULLABLE **Migration**: ALTER `subscriptions.stripe_customer_id` to drop NOT NULL constraint: ```sql ALTER TABLE subscriptions ALTER COLUMN stripe_customer_id DROP NOT NULL; ``` - Clean up existing `admin_override_*` values to NULL - Remove the `resolveStripeCustomerId()` lazy resolution method - When user upgrades from admin-set tier, create Stripe customer directly in the upgrade flow - Webhook handlers already use `findByStripeCustomerId()` - NULL records will never match (correct behavior) ### 2. Make `syncTierToUserProfile()` Blocking Change from non-blocking (error swallowed) to blocking (throw on failure): ```typescript private async syncTierToUserProfile(userId: string, tier: SubscriptionTier): Promise<void> { await this.userProfileRepository.updateSubscriptionTier(userId, tier); logger.info('Subscription tier synced to user profile', { userId, tier }); // Errors now propagate to caller } ``` This is safe because: - Single-row UPDATE on indexed column takes <1ms - No Stripe webhook timeout risk - Prevents tier drift in ALL code paths (upgrades, downgrades, webhooks) ### 3. Clean Admin Override Flow Remove `createForAdminOverride()` placeholder logic. The admin flow becomes: 1. Check if subscription record exists 2. If not, create with `stripe_customer_id = NULL`, `stripe_subscription_id = NULL` 3. Set tier and status atomically in transaction (both tables) 4. When user later pays via Stripe, upgrade flow creates the Stripe customer ## Files Affected **Backend**: - `backend/src/features/subscriptions/data/subscriptions.repository.ts` - Remove `createForAdminOverride()`, update queries for nullable stripe_customer_id - `backend/src/features/subscriptions/domain/subscriptions.service.ts` - Remove `resolveStripeCustomerId()`, update `adminOverrideTier()`, make `syncTierToUserProfile()` blocking - `backend/src/features/subscriptions/api/subscriptions.controller.ts` - Update getInvoices/payment-method to check for null instead of prefix - `backend/src/features/subscriptions/migrations/` - New migration for nullable stripe_customer_id **Frontend** (minor): - `frontend/src/features/subscription/pages/SubscriptionPage.tsx` - Conditional rendering for invoices/payment when no Stripe customer ## Acceptance Criteria - [ ] `stripe_customer_id` column is NULLABLE (migration applied) - [ ] All existing `admin_override_*` values migrated to NULL - [ ] `resolveStripeCustomerId()` method removed - [ ] `createForAdminOverride()` creates records with NULL stripe_customer_id - [ ] Admin can set any tier (free/pro/enterprise) without Stripe interaction - [ ] User with admin-set tier can later upgrade via Stripe payment (new Stripe customer created in upgrade flow) - [ ] User with admin-set tier that is downgraded can still pay for upgrade via Stripe - [ ] `syncTierToUserProfile()` throws on failure (blocking) - [ ] Webhook handlers continue to function (NULL stripe_customer_id records are never matched) - [ ] Invoices endpoint returns `[]` for users without Stripe customer - [ ] Payment method endpoint gracefully handles NULL stripe_customer_id - [ ] All linters pass - [ ] All tests pass - [ ] Mobile + desktop responsive validation ## Test Plan 1. **Admin Override Flow**: - Admin sets user from Free to Pro - verify both tables updated, no placeholder in stripe_customer_id - Admin sets user from Pro to Free - verify downgrade, vehicle selection prompt if needed - Admin sets user from Free to Enterprise - verify upgrade 2. **Stripe Upgrade After Admin Override**: - Admin sets user to Pro (no Stripe) - Admin downgrades to Free - User pays for Pro via Stripe - verify new Stripe customer created, subscription active - Verify webhooks work for this user going forward 3. **Blocking Sync**: - Verify tier changes fail atomically if user_profiles update fails - Verify webhook-triggered tier changes sync correctly 4. **Regression**: - Normal Stripe upgrade flow (no admin involvement) still works - Normal Stripe downgrade flow still works - Webhook processing still works - Grace period job still processes correctly ## Decision Critic Results Decisions stress-tested via Decision Critic skill. Key findings: - Eliminating `admin_override_` entirely: **VERIFIED** - NULLABLE stripe_customer_id correctly isolates admin subscriptions from Stripe webhooks - Blocking sync: **VERIFIED** - Safe, <1ms operation, prevents drift in all paths - Manual tier change only: **VERIFIED** - Seamless upgrade path preserved - UUID migration: **SEPARATED** into independent issue (not bundled here to reduce risk)
egullickson added the
status
backlog
type
feature
 labels 2026-02-16 14:42:08 +00:00
egullickson added
status
in-progress
 and removed
status
backlog
 labels 2026-02-16 14:48:32 +00:00
egullickson added this to the Sprint 2026-02-02 milestone 2026-02-16 14:48:33 +00:00
egullickson commented 2026-02-16 14:52:47 +00:00
Author
Owner
Copy Link
Copy Source

Plan: Clean up subscription admin override and Stripe integration

Phase: Planning | Agent: Orchestrator | Status: APPROVED

Codebase Analysis Summary

Analyzed all affected files across backend and frontend. Key findings:

Repository (subscriptions.repository.ts):

  • createForAdminOverride() (lines 576-611) generates admin_override_{userId}_{timestamp} placeholder
  • mapSubscriptionRow() (lines 622-638) maps stripe_customer_id as always-present string
  • Subscription type has stripeCustomerId: string (not optional)
  • findByStripeCustomerId() (lines 90-113) does exact match - NULL records never match (correct)

Service (subscriptions.service.ts):

  • resolveStripeCustomerId() (lines 173-198) - lazy Stripe customer creation on admin_override_ detection
  • syncTierToUserProfile() (lines 804-816) - swallows all errors (line 814 comment: "Don't throw")
  • Called from 4 locations: upgradeSubscription (253), downgradeSubscription (453), handleSubscriptionUpdated (607), handleSubscriptionDeleted (642)
  • adminOverrideTier() (lines 899-960) uses explicit transaction for atomic dual-table update
  • getInvoices() (line 981) checks admin_override_ prefix
  • upgradeSubscription() and updatePaymentMethod() call resolveStripeCustomerId()

Controller (subscriptions.controller.ts):

  • 8 endpoints, all authenticated. Controller delegates to service - admin_override_ checks are in service layer
  • cancel/reactivate endpoints have NO admin_override_ guards (risk of Stripe errors for admin-set users)

Migration (001_subscriptions_tables.sql):

  • stripe_customer_id VARCHAR(255) UNIQUE NOT NULL (line 29)
  • Index exists: idx_subscriptions_stripe_customer_id
  • Migration numbering: {3-digit}_{snake_case}.sql
  • PostgreSQL UNIQUE allows multiple NULLs (safe for this refactor)

Frontend: Zero references to admin_override_ or stripeCustomerId in UI. Already handles empty invoices gracefully. Type-only change needed.

Grace Period Job (grace-period.job.ts): Does NOT reference stripe_customer_id or call syncTierToUserProfile(). Uses inline SQL in transaction. Admin override records (stripe_subscription_id = NULL) are naturally excluded from expiration logic.

Admin Controller (admin/api/users.controller.ts): Calls adminOverrideTier() at line 288. Clean delegation, no changes needed.

Feature Context

Admin override subscriptions enable manual tier assignment without Stripe billing. Use cases: internal testing accounts, partner/sponsor accounts, emergency access grants, transition period before payment setup. Admin overrides coexist with paid subscriptions in a single table, enabling a seamless upgrade path when the user adds a payment method.

Planning Context

Decision Log

Decision Reasoning Chain Verification
NULLABLE stripe_customer_id admin_override_ placeholder violates column semantics -> caused bug #58 -> every code path must check prefix -> NULL correctly represents "no Stripe customer" -> eliminates all prefix checks Decision Critic: VERIFIED. PostgreSQL UNIQUE allows multiple NULLs. Webhook handlers use findByStripeCustomerId() which returns null for no match - NULL records correctly isolated.
Blocking syncTierToUserProfile() Non-blocking swallows errors silently -> user_profiles tier drifts from subscriptions.tier -> auth context reads stale tier -> user sees wrong features Decision Critic: VERIFIED. Single-row UPDATE on indexed column <1ms. No network calls in sync path. No webhook timeout risk. Only failure mode (DB connection loss) would also fail the preceding subscription update.
Extract ensureStripeCustomer() method Inline Stripe customer creation in upgrade + updatePaymentMethod is duplicated -> extract private method with cleanup logic for orphaned Stripe customers if repository.update() fails QR plan-code: ACCEPTED. Addresses RULE 0 (failure handling) and RULE 2 (duplication).
cancel/reactivate throw BadRequestError for NULL Admin-set tiers have no Stripe subscription to cancel/reactivate -> calling Stripe API would fail with cryptic error -> explicit error message guides user to correct action Aligns with existing pattern: other endpoints already return clear errors for missing resources. Guard placed in service layer.

Rejected Alternatives

Alternative Why Rejected
UUID migration for subscription IDs Large scope, independent risk profile. Separated into independent issue to reduce blast radius.
Separate admin_subscriptions table Over-engineers the solution. Same table with NULL stripe fields cleanly separates the two modes. Admin users can transition to Stripe seamlessly.
Keep non-blocking sync with retry Adds complexity. The sync operation is <1ms on indexed column. Blocking is simpler and prevents drift.
Non-blocking sync with escalated logging QR suggested keeping try-catch with alert flags. Rejected: the only failure mode (DB down) would also fail the preceding subscription update, making the non-blocking path unreachable. Blocking is simpler and correct.

Constraints & Assumptions

  • PostgreSQL UNIQUE constraint allows multiple NULL values (SQL standard behavior, verified)
  • stripe_subscription_id is already NULLABLE in the schema - this refactor aligns stripe_customer_id to the same pattern
  • No GET payment method endpoint exists - only PUT (updatePaymentMethod). Confirmed by codebase analysis.
  • Frontend does not render stripeCustomerId anywhere - type change only, no UI impact
  • Grace period job uses inline SQL, not syncTierToUserProfile() - no impact from blocking change. Admin override records (stripe_subscription_id = NULL) are naturally excluded from expiration.
  • API contract change: GET /api/subscriptions response field stripeCustomerId changes from always-string (including admin_override_* values) to string | null. Frontend does not display this field, so no UI impact. Single-tenant application with no external API consumers.
  • Race condition between upgrade and webhook: Pre-existing in resolveStripeCustomerId(). Webhook handlers already handle "not found" gracefully. Stripe webhooks are delayed by seconds. Not introduced or worsened by this refactor.
  • Tests directory for subscriptions feature is empty - no existing tests to update when removing resolveStripeCustomerId().

Milestone 1: Database migration + type definitions

Sub-issue: #207 refactor: Database migration for nullable stripe_customer_id (#205)

Files:

  • backend/src/features/subscriptions/migrations/002_nullable_stripe_customer_id.sql (NEW)
  • backend/src/features/subscriptions/domain/subscriptions.types.ts
  • frontend/src/features/subscription/types/subscription.types.ts

Changes:

  1. New migration 002_nullable_stripe_customer_id.sql:

    -- Drop NOT NULL constraint on stripe_customer_id
ALTER TABLE subscriptions ALTER COLUMN stripe_customer_id DROP NOT NULL;

-- Clean up existing admin_override_ placeholder values
UPDATE subscriptions SET stripe_customer_id = NULL 
WHERE stripe_customer_id LIKE 'admin_override_%';
    • UNIQUE constraint preserved (PostgreSQL allows multiple NULLs)
    • Index preserved for query performance on non-NULL values

  2. Backend type (subscriptions.types.ts):

    • Subscription interface: change stripeCustomerId: string to stripeCustomerId: string | null
    • SubscriptionResponse type: change stripeCustomerId: string to stripeCustomerId: string | null
    • UpdateSubscriptionData type: change stripeCustomerId?: string to stripeCustomerId?: string | null

  3. Frontend type (subscription.types.ts):

    • Change stripeCustomerId: string to stripeCustomerId: string | null

Tests: Verify migration runs without errors. Verify admin_override_* values become NULL after migration. Verify type compilation in both backend and frontend.

Acceptance criteria: Migration runs cleanly, all admin_override_* values become NULL, all types (Subscription, SubscriptionResponse, UpdateSubscriptionData) compile with nullable stripeCustomerId.

Milestone 2: Repository layer updates

Sub-issue: #208 refactor: Repository updates for nullable stripe_customer_id (#205)

Files:

  • backend/src/features/subscriptions/data/subscriptions.repository.ts

Changes:

  1. createForAdminOverride() (lines 576-611):

    • Remove placeholder generation (admin_override_{userId}_{timestamp})
    • INSERT with stripe_customer_id = NULL instead

  2. mapSubscriptionRow() (lines 622-638):

    • Change line 626 to use nullish coalescing: stripeCustomerId: row.stripe_customer_id ?? null
    • This avoids coercing empty strings and is consistent with the null type

  3. create() method (lines 30-57):

    • Make stripeCustomerId optional in the parameter type
    • Default to NULL in INSERT when not provided

Tests: Verify createForAdminOverride() inserts NULL for stripe_customer_id. Verify mapSubscriptionRow() returns null (not undefined) for NULL db values. Verify create() works with and without stripeCustomerId.

Acceptance criteria: Admin override creates records with NULL stripe_customer_id. Mapper handles NULL correctly using ??. Existing Stripe-based creates still work.

Milestone 3: Service - admin override + sync hardening

Sub-issue: #209 refactor: Remove resolveStripeCustomerId and harden tier sync (#205)

Files:

  • backend/src/features/subscriptions/domain/subscriptions.service.ts

Changes:

  1. Delete resolveStripeCustomerId() (lines 173-198):

    • Remove the entire method
    • Replacement: ensureStripeCustomer() in Milestone 4

  2. Update adminOverrideTier() (lines 899-960):

    • The method already uses a transaction and syncs both tables atomically
    • Only change: createForAdminOverride() creates with NULL (handled by Milestone 2)
    • No logic changes needed in this method - it delegates correctly

  3. Make syncTierToUserProfile() blocking (lines 804-816):

    • Remove the try-catch that swallows errors
    • Let errors propagate to caller
    // After:
private async syncTierToUserProfile(userId: string, tier: SubscriptionTier): Promise<void> {
  await this.userProfileRepository.updateSubscriptionTier(userId, tier);
  logger.info('Subscription tier synced to user profile', { userId, tier });
}
    • Safe because: single-row UPDATE on indexed column (<1ms), no webhook timeout risk
    • All 4 callers (upgradeSubscription, downgradeSubscription, handleSubscriptionUpdated, handleSubscriptionDeleted) fail atomically if sync fails

Tests: Verify resolveStripeCustomerId() is fully removed (no references, no tests - tests dir is empty). Verify syncTierToUserProfile() propagates errors. Verify adminOverrideTier() transaction commits with NULL stripe_customer_id.

Acceptance criteria: resolveStripeCustomerId() deleted. syncTierToUserProfile() is blocking (throws on failure). Admin override flow works with NULL stripe_customer_id.

Milestone 4: Service - Stripe flows + null handling

Sub-issue: #210 refactor: Update Stripe flows for nullable stripe_customer_id (#205)

Files:

  • backend/src/features/subscriptions/domain/subscriptions.service.ts
  • backend/src/features/subscriptions/api/subscriptions.controller.ts (comment cleanup only)

Changes:

  1. Extract ensureStripeCustomer() private method (replaces resolveStripeCustomerId()):

    // Create or return existing Stripe customer for a subscription.
// Handles cleanup of orphaned Stripe customer if DB update fails.
private async ensureStripeCustomer(
  subscription: Subscription,
  email: string
): Promise<string> {
  if (subscription.stripeCustomerId) {
    return subscription.stripeCustomerId;
  }

  const stripeCustomer = await this.stripeClient.createCustomer(email);
  try {
    await this.repository.update(subscription.id, { stripeCustomerId: stripeCustomer.id });
    logger.info('Created Stripe customer for subscription', {
      subscriptionId: subscription.id,
      stripeCustomerId: stripeCustomer.id,
    });
    return stripeCustomer.id;
  } catch (error) {
    // Attempt cleanup of orphaned Stripe customer
    try {
      await this.stripeClient.deleteCustomer(stripeCustomer.id);
      logger.warn('Rolled back orphaned Stripe customer after DB update failure', {
        stripeCustomerId: stripeCustomer.id,
      });
    } catch (cleanupError: any) {
      logger.error('Failed to cleanup orphaned Stripe customer', {
        stripeCustomerId: stripeCustomer.id,
        cleanupError: cleanupError.message,
      });
    }
    throw error;
  }
}

  2. upgradeSubscription() (lines 203-280):

    • Replace resolveStripeCustomerId() call (line 220) with:
    const stripeCustomerId = await this.ensureStripeCustomer(currentSubscription, email);

  3. updatePaymentMethod() (lines 965-973):

    • Same pattern: replace resolveStripeCustomerId() call with ensureStripeCustomer()

  4. getInvoices() (line 981):

    • Change from prefix check to null check:
    if (!subscription?.stripeCustomerId) {
  return [];
}

  5. Edge case hardening for cancelSubscription() and reactivateSubscription() (service layer):

    • Add null guard in service methods: if stripeCustomerId is null, throw BadRequestError with message: "Cannot cancel/reactivate subscription without active Stripe billing"

  6. Controller comment cleanup: Remove any stale comments in subscriptions.controller.ts referencing admin_override_ pattern

Tests: Verify ensureStripeCustomer() creates Stripe customer when stripeCustomerId is null. Verify ensureStripeCustomer() cleans up orphaned customer on DB failure. Verify ensureStripeCustomer() returns existing customer when stripeCustomerId is set. Verify getInvoices() returns [] for null stripeCustomerId. Verify cancelSubscription() throws BadRequestError for null stripeCustomerId. Verify reactivateSubscription() throws BadRequestError for null stripeCustomerId.

Mobile + Desktop Validation: Verify SubscriptionPage and SubscriptionMobileScreen render correctly at 320px, 768px, and 1920px viewports when user has NULL stripe_customer_id (no invoices section, no payment method errors, tier display correct).

Acceptance criteria: ensureStripeCustomer() extracted with orphan cleanup. Upgrade from admin-set tier creates real Stripe customer. Payment method update works for admin-set users. Invoices return [] for null stripe_customer_id. Cancel/reactivate throw BadRequestError for null stripeCustomerId (service layer). Controller comments updated. Mobile and desktop render correctly. All linters pass. All tests pass.

Invisible Knowledge

Feature Context: Admin override subscriptions enable manual tier assignment without Stripe billing. Use cases: testing accounts, partner accounts, emergency access. Admin overrides coexist with paid subscriptions in a single table, enabling seamless upgrade when user adds payment.

Admin-to-Stripe Transition Path: Admin-set subscriptions (stripe_customer_id = NULL) live in the same subscriptions table as Stripe-managed subscriptions. When a user with an admin-set tier upgrades via Stripe, ensureStripeCustomer() creates a real Stripe customer and updates the record in-place. This single-table design enables seamless transition without data migration.

Webhook Isolation: Stripe webhooks use findByStripeCustomerId() to locate subscriptions. Since admin-set subscriptions have NULL stripe_customer_id, they are naturally invisible to webhook handlers. No additional guards needed in webhook code paths.

Dual-Table Sync Pattern: The subscription tier is stored in two tables: subscriptions.tier (billing source of truth) and user_profiles.subscription_tier (auth context cache). All non-admin flows use syncTierToUserProfile() which is blocking and throws on failure. The admin flow (adminOverrideTier()) uses an explicit transaction to update both tables atomically.

NULL vs Sentinel Values: PostgreSQL UNIQUE constraint allows multiple NULLs but fails on duplicate sentinel strings. NULL is the database-native representation of "no value" vs. a string hack (admin_override_*) that violates type semantics and requires prefix checks in every code path. This was the root cause of bug #58.

Dependency Chain

Milestone 1 (Migration + Types)
    |
Milestone 2 (Repository)
    |
Milestone 3 (Service - Delete resolve + Harden sync)
    |
Milestone 4 (Service - ensureStripeCustomer + null handling)

Risk Assessment

Risk Mitigation
Migration on production data Migration is additive (DROP NOT NULL), reversible. UPDATE is targeted.
Multiple NULLs in UNIQUE column PostgreSQL allows multiple NULLs in UNIQUE - verified.
Blocking sync increases failure surface Single-row indexed UPDATE <1ms. Only failure mode (DB down) also fails preceding operations.
Orphaned Stripe customer on DB failure ensureStripeCustomer() has cleanup logic - attempts deleteCustomer() on repository failure.
Cancel/reactivate for admin-set users Throw explicit BadRequestError with clear message (service layer).
Grace period job Does NOT use stripe_customer_id - admin records naturally excluded.
Webhook handlers Use findByStripeCustomerId() - NULL records never match, correct isolation.
API contract change stripeCustomerId goes from always-string to nullable. Single-tenant, no external consumers.
Pre-existing webhook race condition Webhook handlers handle "not found" gracefully. Not introduced by this refactor.

Files NOT Changed (Confirmed Safe)

  • backend/src/features/subscriptions/jobs/grace-period.job.ts - No stripe_customer_id references
  • backend/src/features/admin/api/users.controller.ts - Clean delegation to adminOverrideTier()
  • backend/src/features/subscriptions/api/subscriptions.routes.ts - Route definitions unchanged
  • backend/src/features/subscriptions/external/stripe/ - Stripe client unchanged
  • All frontend pages/components except type definition - No admin_override_ in UI

Review History

Review Verdict Key Changes
QR plan-completeness PASS_WITH_CONCERNS Added: Decision Log, Invisible Knowledge, Tests per milestone, Mobile validation
TW plan-scrub MINOR_EDITS Fixed: ?? operator, cancel/reactivate location, UpdateSubscriptionData type, temporal contamination, API contract change
QR plan-code NEEDS_CHANGES Added: ensureStripeCustomer() with cleanup. Acknowledged: pre-existing race condition. Disagreed: non-blocking sync (Decision Critic verified).
QR plan-docs PASS_WITH_CONCERNS Added: NULL vs Sentinel Knowledge, Feature Context, grace period implications

Verdict: APPROVED | Next: Create branch and begin execution

## Plan: Clean up subscription admin override and Stripe integration **Phase**: Planning | **Agent**: Orchestrator | **Status**: APPROVED --- ### Codebase Analysis Summary Analyzed all affected files across backend and frontend. Key findings: **Repository** (`subscriptions.repository.ts`): - `createForAdminOverride()` (lines 576-611) generates `admin_override_{userId}_{timestamp}` placeholder - `mapSubscriptionRow()` (lines 622-638) maps `stripe_customer_id` as always-present string - `Subscription` type has `stripeCustomerId: string` (not optional) - `findByStripeCustomerId()` (lines 90-113) does exact match - NULL records never match (correct) **Service** (`subscriptions.service.ts`): - `resolveStripeCustomerId()` (lines 173-198) - lazy Stripe customer creation on admin_override_ detection - `syncTierToUserProfile()` (lines 804-816) - **swallows all errors** (line 814 comment: "Don't throw") - Called from 4 locations: upgradeSubscription (253), downgradeSubscription (453), handleSubscriptionUpdated (607), handleSubscriptionDeleted (642) - `adminOverrideTier()` (lines 899-960) uses explicit transaction for atomic dual-table update - `getInvoices()` (line 981) checks `admin_override_` prefix - `upgradeSubscription()` and `updatePaymentMethod()` call `resolveStripeCustomerId()` **Controller** (`subscriptions.controller.ts`): - 8 endpoints, all authenticated. Controller delegates to service - admin_override_ checks are in service layer - `cancel`/`reactivate` endpoints have NO admin_override_ guards (risk of Stripe errors for admin-set users) **Migration** (`001_subscriptions_tables.sql`): - `stripe_customer_id VARCHAR(255) UNIQUE NOT NULL` (line 29) - Index exists: `idx_subscriptions_stripe_customer_id` - Migration numbering: `{3-digit}_{snake_case}.sql` - PostgreSQL UNIQUE allows multiple NULLs (safe for this refactor) **Frontend**: Zero references to `admin_override_` or `stripeCustomerId` in UI. Already handles empty invoices gracefully. Type-only change needed. **Grace Period Job** (`grace-period.job.ts`): Does NOT reference `stripe_customer_id` or call `syncTierToUserProfile()`. Uses inline SQL in transaction. Admin override records (stripe_subscription_id = NULL) are naturally excluded from expiration logic. **Admin Controller** (`admin/api/users.controller.ts`): Calls `adminOverrideTier()` at line 288. Clean delegation, no changes needed. --- ### Feature Context Admin override subscriptions enable manual tier assignment without Stripe billing. Use cases: internal testing accounts, partner/sponsor accounts, emergency access grants, transition period before payment setup. Admin overrides coexist with paid subscriptions in a single table, enabling a seamless upgrade path when the user adds a payment method. --- ### Planning Context #### Decision Log | Decision | Reasoning Chain | Verification | |----------|----------------|--------------| | NULLABLE stripe_customer_id | `admin_override_` placeholder violates column semantics -> caused bug #58 -> every code path must check prefix -> NULL correctly represents "no Stripe customer" -> eliminates all prefix checks | Decision Critic: VERIFIED. PostgreSQL UNIQUE allows multiple NULLs. Webhook handlers use findByStripeCustomerId() which returns null for no match - NULL records correctly isolated. | | Blocking syncTierToUserProfile() | Non-blocking swallows errors silently -> user_profiles tier drifts from subscriptions.tier -> auth context reads stale tier -> user sees wrong features | Decision Critic: VERIFIED. Single-row UPDATE on indexed column <1ms. No network calls in sync path. No webhook timeout risk. Only failure mode (DB connection loss) would also fail the preceding subscription update. | | Extract ensureStripeCustomer() method | Inline Stripe customer creation in upgrade + updatePaymentMethod is duplicated -> extract private method with cleanup logic for orphaned Stripe customers if repository.update() fails | QR plan-code: ACCEPTED. Addresses RULE 0 (failure handling) and RULE 2 (duplication). | | cancel/reactivate throw BadRequestError for NULL | Admin-set tiers have no Stripe subscription to cancel/reactivate -> calling Stripe API would fail with cryptic error -> explicit error message guides user to correct action | Aligns with existing pattern: other endpoints already return clear errors for missing resources. Guard placed in service layer. | #### Rejected Alternatives | Alternative | Why Rejected | |-------------|-------------| | UUID migration for subscription IDs | Large scope, independent risk profile. Separated into independent issue to reduce blast radius. | | Separate admin_subscriptions table | Over-engineers the solution. Same table with NULL stripe fields cleanly separates the two modes. Admin users can transition to Stripe seamlessly. | | Keep non-blocking sync with retry | Adds complexity. The sync operation is <1ms on indexed column. Blocking is simpler and prevents drift. | | Non-blocking sync with escalated logging | QR suggested keeping try-catch with alert flags. Rejected: the only failure mode (DB down) would also fail the preceding subscription update, making the non-blocking path unreachable. Blocking is simpler and correct. | #### Constraints & Assumptions - PostgreSQL UNIQUE constraint allows multiple NULL values (SQL standard behavior, verified) - `stripe_subscription_id` is already NULLABLE in the schema - this refactor aligns `stripe_customer_id` to the same pattern - No GET payment method endpoint exists - only PUT (updatePaymentMethod). Confirmed by codebase analysis. - Frontend does not render `stripeCustomerId` anywhere - type change only, no UI impact - Grace period job uses inline SQL, not syncTierToUserProfile() - no impact from blocking change. Admin override records (stripe_subscription_id = NULL) are naturally excluded from expiration. - **API contract change**: `GET /api/subscriptions` response field `stripeCustomerId` changes from always-string (including `admin_override_*` values) to `string | null`. Frontend does not display this field, so no UI impact. Single-tenant application with no external API consumers. - Race condition between upgrade and webhook: Pre-existing in `resolveStripeCustomerId()`. Webhook handlers already handle "not found" gracefully. Stripe webhooks are delayed by seconds. Not introduced or worsened by this refactor. - Tests directory for subscriptions feature is empty - no existing tests to update when removing `resolveStripeCustomerId()`. --- ### Milestone 1: Database migration + type definitions **Sub-issue**: #207 `refactor: Database migration for nullable stripe_customer_id (#205)` **Files**: - `backend/src/features/subscriptions/migrations/002_nullable_stripe_customer_id.sql` (NEW) - `backend/src/features/subscriptions/domain/subscriptions.types.ts` - `frontend/src/features/subscription/types/subscription.types.ts` **Changes**: 1. **New migration** `002_nullable_stripe_customer_id.sql`: ```sql -- Drop NOT NULL constraint on stripe_customer_id ALTER TABLE subscriptions ALTER COLUMN stripe_customer_id DROP NOT NULL; -- Clean up existing admin_override_ placeholder values UPDATE subscriptions SET stripe_customer_id = NULL WHERE stripe_customer_id LIKE 'admin_override_%'; ``` - UNIQUE constraint preserved (PostgreSQL allows multiple NULLs) - Index preserved for query performance on non-NULL values 2. **Backend type** (`subscriptions.types.ts`): - `Subscription` interface: change `stripeCustomerId: string` to `stripeCustomerId: string | null` - `SubscriptionResponse` type: change `stripeCustomerId: string` to `stripeCustomerId: string | null` - `UpdateSubscriptionData` type: change `stripeCustomerId?: string` to `stripeCustomerId?: string | null` 3. **Frontend type** (`subscription.types.ts`): - Change `stripeCustomerId: string` to `stripeCustomerId: string | null` **Tests**: Verify migration runs without errors. Verify admin_override_* values become NULL after migration. Verify type compilation in both backend and frontend. **Acceptance criteria**: Migration runs cleanly, all admin_override_* values become NULL, all types (Subscription, SubscriptionResponse, UpdateSubscriptionData) compile with nullable stripeCustomerId. --- ### Milestone 2: Repository layer updates **Sub-issue**: #208 `refactor: Repository updates for nullable stripe_customer_id (#205)` **Files**: - `backend/src/features/subscriptions/data/subscriptions.repository.ts` **Changes**: 1. **`createForAdminOverride()`** (lines 576-611): - Remove placeholder generation (`admin_override_{userId}_{timestamp}`) - INSERT with `stripe_customer_id = NULL` instead 2. **`mapSubscriptionRow()`** (lines 622-638): - Change line 626 to use nullish coalescing: `stripeCustomerId: row.stripe_customer_id ?? null` - This avoids coercing empty strings and is consistent with the null type 3. **`create()` method** (lines 30-57): - Make `stripeCustomerId` optional in the parameter type - Default to NULL in INSERT when not provided **Tests**: Verify createForAdminOverride() inserts NULL for stripe_customer_id. Verify mapSubscriptionRow() returns null (not undefined) for NULL db values. Verify create() works with and without stripeCustomerId. **Acceptance criteria**: Admin override creates records with NULL stripe_customer_id. Mapper handles NULL correctly using `??`. Existing Stripe-based creates still work. --- ### Milestone 3: Service - admin override + sync hardening **Sub-issue**: #209 `refactor: Remove resolveStripeCustomerId and harden tier sync (#205)` **Files**: - `backend/src/features/subscriptions/domain/subscriptions.service.ts` **Changes**: 1. **Delete `resolveStripeCustomerId()`** (lines 173-198): - Remove the entire method - Replacement: `ensureStripeCustomer()` in Milestone 4 2. **Update `adminOverrideTier()`** (lines 899-960): - The method already uses a transaction and syncs both tables atomically - Only change: `createForAdminOverride()` creates with NULL (handled by Milestone 2) - No logic changes needed in this method - it delegates correctly 3. **Make `syncTierToUserProfile()` blocking** (lines 804-816): - Remove the try-catch that swallows errors - Let errors propagate to caller ```typescript // After: private async syncTierToUserProfile(userId: string, tier: SubscriptionTier): Promise<void> { await this.userProfileRepository.updateSubscriptionTier(userId, tier); logger.info('Subscription tier synced to user profile', { userId, tier }); } ``` - Safe because: single-row UPDATE on indexed column (<1ms), no webhook timeout risk - All 4 callers (upgradeSubscription, downgradeSubscription, handleSubscriptionUpdated, handleSubscriptionDeleted) fail atomically if sync fails **Tests**: Verify resolveStripeCustomerId() is fully removed (no references, no tests - tests dir is empty). Verify syncTierToUserProfile() propagates errors. Verify adminOverrideTier() transaction commits with NULL stripe_customer_id. **Acceptance criteria**: `resolveStripeCustomerId()` deleted. `syncTierToUserProfile()` is blocking (throws on failure). Admin override flow works with NULL stripe_customer_id. --- ### Milestone 4: Service - Stripe flows + null handling **Sub-issue**: #210 `refactor: Update Stripe flows for nullable stripe_customer_id (#205)` **Files**: - `backend/src/features/subscriptions/domain/subscriptions.service.ts` - `backend/src/features/subscriptions/api/subscriptions.controller.ts` (comment cleanup only) **Changes**: 1. **Extract `ensureStripeCustomer()` private method** (replaces `resolveStripeCustomerId()`): ```typescript // Create or return existing Stripe customer for a subscription. // Handles cleanup of orphaned Stripe customer if DB update fails. private async ensureStripeCustomer( subscription: Subscription, email: string ): Promise<string> { if (subscription.stripeCustomerId) { return subscription.stripeCustomerId; } const stripeCustomer = await this.stripeClient.createCustomer(email); try { await this.repository.update(subscription.id, { stripeCustomerId: stripeCustomer.id }); logger.info('Created Stripe customer for subscription', { subscriptionId: subscription.id, stripeCustomerId: stripeCustomer.id, }); return stripeCustomer.id; } catch (error) { // Attempt cleanup of orphaned Stripe customer try { await this.stripeClient.deleteCustomer(stripeCustomer.id); logger.warn('Rolled back orphaned Stripe customer after DB update failure', { stripeCustomerId: stripeCustomer.id, }); } catch (cleanupError: any) { logger.error('Failed to cleanup orphaned Stripe customer', { stripeCustomerId: stripeCustomer.id, cleanupError: cleanupError.message, }); } throw error; } } ``` 2. **`upgradeSubscription()`** (lines 203-280): - Replace `resolveStripeCustomerId()` call (line 220) with: ```typescript const stripeCustomerId = await this.ensureStripeCustomer(currentSubscription, email); ``` 3. **`updatePaymentMethod()`** (lines 965-973): - Same pattern: replace `resolveStripeCustomerId()` call with `ensureStripeCustomer()` 4. **`getInvoices()`** (line 981): - Change from prefix check to null check: ```typescript if (!subscription?.stripeCustomerId) { return []; } ``` 5. **Edge case hardening** for `cancelSubscription()` and `reactivateSubscription()` (service layer): - Add null guard in service methods: if `stripeCustomerId` is null, throw BadRequestError with message: "Cannot cancel/reactivate subscription without active Stripe billing" 6. **Controller comment cleanup**: Remove any stale comments in `subscriptions.controller.ts` referencing `admin_override_` pattern **Tests**: Verify ensureStripeCustomer() creates Stripe customer when stripeCustomerId is null. Verify ensureStripeCustomer() cleans up orphaned customer on DB failure. Verify ensureStripeCustomer() returns existing customer when stripeCustomerId is set. Verify getInvoices() returns [] for null stripeCustomerId. Verify cancelSubscription() throws BadRequestError for null stripeCustomerId. Verify reactivateSubscription() throws BadRequestError for null stripeCustomerId. **Mobile + Desktop Validation**: Verify SubscriptionPage and SubscriptionMobileScreen render correctly at 320px, 768px, and 1920px viewports when user has NULL stripe_customer_id (no invoices section, no payment method errors, tier display correct). **Acceptance criteria**: `ensureStripeCustomer()` extracted with orphan cleanup. Upgrade from admin-set tier creates real Stripe customer. Payment method update works for admin-set users. Invoices return `[]` for null stripe_customer_id. Cancel/reactivate throw BadRequestError for null stripeCustomerId (service layer). Controller comments updated. Mobile and desktop render correctly. All linters pass. All tests pass. --- ### Invisible Knowledge **Feature Context**: Admin override subscriptions enable manual tier assignment without Stripe billing. Use cases: testing accounts, partner accounts, emergency access. Admin overrides coexist with paid subscriptions in a single table, enabling seamless upgrade when user adds payment. **Admin-to-Stripe Transition Path**: Admin-set subscriptions (stripe_customer_id = NULL) live in the same `subscriptions` table as Stripe-managed subscriptions. When a user with an admin-set tier upgrades via Stripe, `ensureStripeCustomer()` creates a real Stripe customer and updates the record in-place. This single-table design enables seamless transition without data migration. **Webhook Isolation**: Stripe webhooks use `findByStripeCustomerId()` to locate subscriptions. Since admin-set subscriptions have NULL stripe_customer_id, they are naturally invisible to webhook handlers. No additional guards needed in webhook code paths. **Dual-Table Sync Pattern**: The subscription tier is stored in two tables: `subscriptions.tier` (billing source of truth) and `user_profiles.subscription_tier` (auth context cache). All non-admin flows use `syncTierToUserProfile()` which is blocking and throws on failure. The admin flow (`adminOverrideTier()`) uses an explicit transaction to update both tables atomically. **NULL vs Sentinel Values**: PostgreSQL UNIQUE constraint allows multiple NULLs but fails on duplicate sentinel strings. NULL is the database-native representation of "no value" vs. a string hack (`admin_override_*`) that violates type semantics and requires prefix checks in every code path. This was the root cause of bug #58. --- ### Dependency Chain ``` Milestone 1 (Migration + Types) | Milestone 2 (Repository) | Milestone 3 (Service - Delete resolve + Harden sync) | Milestone 4 (Service - ensureStripeCustomer + null handling) ``` ### Risk Assessment | Risk | Mitigation | |------|-----------| | Migration on production data | Migration is additive (DROP NOT NULL), reversible. UPDATE is targeted. | | Multiple NULLs in UNIQUE column | PostgreSQL allows multiple NULLs in UNIQUE - verified. | | Blocking sync increases failure surface | Single-row indexed UPDATE <1ms. Only failure mode (DB down) also fails preceding operations. | | Orphaned Stripe customer on DB failure | ensureStripeCustomer() has cleanup logic - attempts deleteCustomer() on repository failure. | | Cancel/reactivate for admin-set users | Throw explicit BadRequestError with clear message (service layer). | | Grace period job | Does NOT use stripe_customer_id - admin records naturally excluded. | | Webhook handlers | Use findByStripeCustomerId() - NULL records never match, correct isolation. | | API contract change | stripeCustomerId goes from always-string to nullable. Single-tenant, no external consumers. | | Pre-existing webhook race condition | Webhook handlers handle "not found" gracefully. Not introduced by this refactor. | ### Files NOT Changed (Confirmed Safe) - `backend/src/features/subscriptions/jobs/grace-period.job.ts` - No stripe_customer_id references - `backend/src/features/admin/api/users.controller.ts` - Clean delegation to adminOverrideTier() - `backend/src/features/subscriptions/api/subscriptions.routes.ts` - Route definitions unchanged - `backend/src/features/subscriptions/external/stripe/` - Stripe client unchanged - All frontend pages/components except type definition - No admin_override_ in UI --- ### Review History | Review | Verdict | Key Changes | |--------|---------|-------------| | QR plan-completeness | PASS_WITH_CONCERNS | Added: Decision Log, Invisible Knowledge, Tests per milestone, Mobile validation | | TW plan-scrub | MINOR_EDITS | Fixed: `??` operator, cancel/reactivate location, UpdateSubscriptionData type, temporal contamination, API contract change | | QR plan-code | NEEDS_CHANGES | Added: ensureStripeCustomer() with cleanup. Acknowledged: pre-existing race condition. Disagreed: non-blocking sync (Decision Critic verified). | | QR plan-docs | PASS_WITH_CONCERNS | Added: NULL vs Sentinel Knowledge, Feature Context, grace period implications | --- *Verdict*: APPROVED | *Next*: Create branch and begin execution
egullickson commented 2026-02-16 14:55:57 +00:00
Author
Owner
Copy Link
Copy Source

QR Review: plan-completeness

Phase: Plan-Review | Agent: Quality Reviewer | Status: PASS_WITH_CONCERNS

VERDICT: PASS_WITH_CONCERNS

Findings

RULE 1 HIGH: Missing Mobile + Desktop Validation in Milestones

  • Location: All milestone sections (1-4)
  • Issue: No milestone explicitly includes responsive validation
  • Suggested Fix: Add to Milestone 4: "Mobile validation: SubscriptionPage renders correctly on 320px, 768px, 1920px viewports when stripe_customer_id is NULL"

RULE 1 HIGH: Missing Test Coverage in Plan

  • Location: All milestones
  • Issue: Zero test specifications across all 4 milestones
  • Suggested Fix: Add Tests section to each milestone

RULE 1 HIGH: Missing Decision Log Section

  • Location: Plan structure
  • Issue: No "Decision Log", "Rejected Alternatives", or "Constraints & Assumptions" sections
  • Suggested Fix: Add Planning Context with decision chains

RULE 1 HIGH: Missing Invisible Knowledge Section

  • Location: Plan structure
  • Issue: No invisible knowledge content for TW agent to generate documentation
  • Suggested Fix: Add section or explicitly state empty

RULE 2 SHOULD_FIX: Incomplete Edge Case Coverage

  • Location: Milestone 4
  • Issue: "Cancel/reactivate handle null gracefully" lacks specifics
  • Suggested Fix: Specify: throw BadRequestError if stripeCustomerId is NULL

RULE 2 SHOULD_FIX: Payment Method Endpoint Ambiguity

  • Location: Milestone 4
  • Issue: GET payment method endpoint not addressed
  • Resolution: Codebase analysis confirmed NO GET payment method endpoint exists. Only PUT (updatePaymentMethod). Finding resolved.

Considered But Not Flagged

  • Dependency chain: Verified correct (M1->M2->M3->M4)
  • File completeness: Thoroughly analyzed with safe-file justifications
  • Sub-issue decomposition: Follows workflow contract 1:1 mapping
  • Risk assessment: Complete with mitigations

Verdict: PASS_WITH_CONCERNS | Next: Revise plan, then continue review cycle

## QR Review: plan-completeness **Phase**: Plan-Review | **Agent**: Quality Reviewer | **Status**: PASS_WITH_CONCERNS --- ### VERDICT: PASS_WITH_CONCERNS ### Findings #### RULE 1 HIGH: Missing Mobile + Desktop Validation in Milestones - **Location**: All milestone sections (1-4) - **Issue**: No milestone explicitly includes responsive validation - **Suggested Fix**: Add to Milestone 4: "Mobile validation: SubscriptionPage renders correctly on 320px, 768px, 1920px viewports when stripe_customer_id is NULL" #### RULE 1 HIGH: Missing Test Coverage in Plan - **Location**: All milestones - **Issue**: Zero test specifications across all 4 milestones - **Suggested Fix**: Add Tests section to each milestone #### RULE 1 HIGH: Missing Decision Log Section - **Location**: Plan structure - **Issue**: No "Decision Log", "Rejected Alternatives", or "Constraints & Assumptions" sections - **Suggested Fix**: Add Planning Context with decision chains #### RULE 1 HIGH: Missing Invisible Knowledge Section - **Location**: Plan structure - **Issue**: No invisible knowledge content for TW agent to generate documentation - **Suggested Fix**: Add section or explicitly state empty #### RULE 2 SHOULD_FIX: Incomplete Edge Case Coverage - **Location**: Milestone 4 - **Issue**: "Cancel/reactivate handle null gracefully" lacks specifics - **Suggested Fix**: Specify: throw BadRequestError if stripeCustomerId is NULL #### RULE 2 SHOULD_FIX: Payment Method Endpoint Ambiguity - **Location**: Milestone 4 - **Issue**: GET payment method endpoint not addressed - **Resolution**: Codebase analysis confirmed NO GET payment method endpoint exists. Only PUT (updatePaymentMethod). Finding resolved. ### Considered But Not Flagged - Dependency chain: Verified correct (M1->M2->M3->M4) - File completeness: Thoroughly analyzed with safe-file justifications - Sub-issue decomposition: Follows workflow contract 1:1 mapping - Risk assessment: Complete with mitigations --- *Verdict*: PASS_WITH_CONCERNS | *Next*: Revise plan, then continue review cycle
egullickson commented 2026-02-16 14:59:08 +00:00
Author
Owner
Copy Link
Copy Source

TW Review: plan-scrub

Phase: Plan-Review | Agent: Technical Writer | Status: MINOR_EDITS

TW VERDICT: MINOR_EDITS

The plan is well-structured with clear decision chains, accurate code references, and sufficient detail for developer implementation. The QR plan-completeness findings were addressed comprehensively. Seven findings below -- all minor precision issues that can be resolved with targeted edits.

Findings

1. Language Imprecision: mapSubscriptionRow NULL Handling

  • Location: Plan comment, Milestone 2, Change #2
  • Issue: The plan says stripeCustomerId: row.stripe_customer_id || null but the current code at line 626 is stripeCustomerId: row.stripe_customer_id (no fallback). The || operator will coerce empty string "" to null, which may be the intent, but this is an implicit behavior change beyond the stated goal. Additionally, the existing mapper uses || undefined for other nullable fields (e.g., stripeSubscriptionId at line 627), creating an inconsistency: some nullable Stripe fields will return undefined, while stripeCustomerId will return null.
  • Suggested Edit: Clarify the mapping strategy explicitly: 
    stripeCustomerId: row.stripe_customer_id ?? null
    Use nullish coalescing (??) instead of logical OR (||) to only coerce null/undefined to null, not empty strings. Also note: consider whether stripeSubscriptionId should follow the same ?? null pattern for consistency (currently || undefined). If intentionally different, state why.

2. Ambiguity: cancel/reactivate Error Handling Location

  • Location: Plan comment, Milestone 4, Change #4
  • Issue: The plan says "Add null guard: if stripeCustomerId is null, throw BadRequestError" but does not specify whether this guard goes in the service methods (cancelSubscription() at line 285, reactivateSubscription() at line 332) or the controller methods. Currently, cancelSubscription() already checks !currentSubscription.stripeSubscriptionId (line 295) and throws new Error(...). The plan's guard is for a different field (stripeCustomerId), and the error type changes from Error to BadRequestError. The sub-issue #210 body says "handle null gracefully" which contradicts the plan comment's "throw BadRequestError."
  • Suggested Edit:
    • Specify the guard goes in the service layer (consistent with existing pattern at line 295).
    • Specify the exact position: add the stripeCustomerId null check before the existing stripeSubscriptionId check.
    • Update sub-issue #210 acceptance criterion from "Cancel/reactivate handle null gracefully" to "Cancel/reactivate throw BadRequestError with message 'Cannot cancel/reactivate subscription without active Stripe billing' when stripeCustomerId is null."

3. Sub-Issue #210 Body Diverges from Plan Comment

  • Location: Sub-issue #210 body vs. Plan comment Milestone 4
  • Issue: The plan comment (revised after QR review) includes detailed code snippets for upgradeSubscription(), explicit BadRequestError specification for cancel/reactivate, mobile+desktop viewport validation, and test specifications. Sub-issue #210's body has none of these -- it still uses the pre-revision text "handle null gracefully" and lacks the tests section and mobile validation section that were added to the plan comment.
  • Suggested Edit: Update sub-issue #210 body to match the plan comment's Milestone 4 section exactly, including:
    • Code snippet for upgradeSubscription() inline logic
    • "throw BadRequestError" instead of "handle null gracefully"
    • Tests section
    • Mobile + Desktop Validation section with viewport widths

4. Missing Specification: repository.update() Call Signature

  • Location: Plan comment, Milestone 4, Change #1 (upgradeSubscription code snippet)
  • Issue: The inline code snippet calls await this.repository.update(currentSubscription.id, { stripeCustomerId }). Looking at the repository update() method (line 144), UpdateSubscriptionData already has stripeCustomerId?: string. However, after Milestone 1, the type changes to string | null. The plan does not state whether UpdateSubscriptionData.stripeCustomerId also needs updating. If the developer follows only the M1 type changes (Subscription interface), they may miss updating UpdateSubscriptionData.
  • Suggested Edit: Add to Milestone 1 type changes: "Update UpdateSubscriptionData.stripeCustomerId from string to string | null to match the Subscription interface change."

5. Temporal Contamination (Minor)

  • Location: Plan comment, multiple locations
  • Issue: Several phrases use temporal language that will age:
    • "is now blocking" (Invisible Knowledge, Dual-Table Sync Pattern) -- after implementation, "now" is meaningless
    • "now creates with NULL" (Milestone 3, Change #2) -- refers to the M2 change that hasn't happened yet at time of reading
    • "now fail atomically" (Milestone 3, Change #3) -- same issue
  • Suggested Edit: Replace temporal references with absolute statements:
    • "is now blocking" -> "is blocking"
    • "now creates with NULL" -> "creates with NULL (per Milestone 2)"
    • "now fail atomically" -> "fail atomically after this change"

6. Imprecise Scope: Controller Comment at Line 223

  • Location: Plan comment, Codebase Analysis Summary, "Controller" section
  • Issue: The plan states "Controller delegates to service - admin_override_ checks are in service layer" and flags "cancel/reactivate endpoints have NO admin_override_ guards." However, the controller at line 223 has an inline comment // Update payment method via service (handles admin_override_ customer IDs) which should be updated during implementation. This stale comment is not called out in any milestone's file changes. Since the controller is listed under "Files NOT Changed (Confirmed Safe)", the developer will skip it.
  • Suggested Edit: Either:
    • (a) Add controller to Milestone 4 file list with a single change: "Remove stale comment referencing admin_override_ at line 223", or
    • (b) Add a note under "Files NOT Changed" acknowledging the stale comment will persist until a separate cleanup

7. Missing Specification: SubscriptionResponse Type Alignment

  • Location: Plan comment, Milestone 1, Change #2
  • Issue: The plan says to update SubscriptionResponse type, but the current SubscriptionResponse (types file line 77) has stripeCustomerId: string (non-nullable). The plan mentions this change but does not address the mapToResponse() method in the service (line 997-1013) which assigns stripeCustomerId: subscription.stripeCustomerId directly. After the type change, SubscriptionResponse.stripeCustomerId becomes string | null, and the mapper will pass through null to the API response. This is technically correct but the plan should explicitly state that the API contract changes: stripeCustomerId field in JSON responses will be null instead of an admin_override_* string.
  • Suggested Edit: Add to Milestone 1: "API contract change: GET /api/subscriptions response will return stripeCustomerId: null for admin-set subscriptions instead of stripeCustomerId: 'admin_override_...'. Frontend already has no dependency on this value -- confirmed by codebase analysis."

Documentation Readiness

Assessment: READY with minor gaps

The plan provides sufficient material for code comments and README generation:

  • Decision Log: All 4 decisions have clear reasoning chains that translate directly to JSDoc comments. The "Reasoning Chain" column format is particularly effective for inline // Decision: ... comments.

  • Invisible Knowledge: The 3 items (Admin-to-Stripe Transition Path, Webhook Isolation, Dual-Table Sync Pattern) are well-written and README-ready. The Dual-Table Sync Pattern item should remove "now" as noted in Finding #5.

  • Code Comment Gap: The plan does not specify what JSDoc or inline comment should replace the deleted resolveStripeCustomerId() method's documentation. The inline Stripe customer creation in upgradeSubscription() and updatePaymentMethod() should carry forward the "why" context. Suggest adding a comment template to Milestone 4:

    // Create Stripe customer for admin-overridden subscription.
// Admin-set subscriptions have NULL stripeCustomerId.
// On first Stripe payment, we create the customer in-place
// rather than using lazy resolution (see Decision Log #3).

  • README Update Scope: The subscriptions feature README (backend/src/features/subscriptions/README.md) should be updated to document the NULL stripeCustomerId semantics. This is not called out in any milestone. Recommend adding a documentation task to Milestone 4 or as a separate follow-up.

Verdict: MINOR_EDITS | Next: Address findings, then QR plan-code -> QR plan-docs

## TW Review: plan-scrub **Phase**: Plan-Review | **Agent**: Technical Writer | **Status**: MINOR_EDITS --- ## TW VERDICT: MINOR_EDITS The plan is well-structured with clear decision chains, accurate code references, and sufficient detail for developer implementation. The QR plan-completeness findings were addressed comprehensively. Seven findings below -- all minor precision issues that can be resolved with targeted edits. --- ## Findings ### 1. Language Imprecision: mapSubscriptionRow NULL Handling - **Location**: Plan comment, Milestone 2, Change #2 - **Issue**: The plan says `stripeCustomerId: row.stripe_customer_id || null` but the current code at line 626 is `stripeCustomerId: row.stripe_customer_id` (no fallback). The `||` operator will coerce empty string `""` to `null`, which may be the intent, but this is an implicit behavior change beyond the stated goal. Additionally, the existing mapper uses `|| undefined` for other nullable fields (e.g., `stripeSubscriptionId` at line 627), creating an inconsistency: some nullable Stripe fields will return `undefined`, while `stripeCustomerId` will return `null`. - **Suggested Edit**: Clarify the mapping strategy explicitly: ``` stripeCustomerId: row.stripe_customer_id ?? null ``` Use nullish coalescing (`??`) instead of logical OR (`||`) to only coerce `null`/`undefined` to `null`, not empty strings. Also note: consider whether `stripeSubscriptionId` should follow the same `?? null` pattern for consistency (currently `|| undefined`). If intentionally different, state why. ### 2. Ambiguity: cancel/reactivate Error Handling Location - **Location**: Plan comment, Milestone 4, Change #4 - **Issue**: The plan says "Add null guard: if `stripeCustomerId` is null, throw BadRequestError" but does not specify whether this guard goes in the **service** methods (`cancelSubscription()` at line 285, `reactivateSubscription()` at line 332) or the **controller** methods. Currently, `cancelSubscription()` already checks `!currentSubscription.stripeSubscriptionId` (line 295) and throws `new Error(...)`. The plan's guard is for a *different* field (`stripeCustomerId`), and the error type changes from `Error` to `BadRequestError`. The sub-issue #210 body says "handle null gracefully" which contradicts the plan comment's "throw BadRequestError." - **Suggested Edit**: - Specify the guard goes in the **service** layer (consistent with existing pattern at line 295). - Specify the exact position: add the `stripeCustomerId` null check *before* the existing `stripeSubscriptionId` check. - Update sub-issue #210 acceptance criterion from "Cancel/reactivate handle null gracefully" to "Cancel/reactivate throw BadRequestError with message 'Cannot cancel/reactivate subscription without active Stripe billing' when stripeCustomerId is null." ### 3. Sub-Issue #210 Body Diverges from Plan Comment - **Location**: Sub-issue #210 body vs. Plan comment Milestone 4 - **Issue**: The plan comment (revised after QR review) includes detailed code snippets for `upgradeSubscription()`, explicit `BadRequestError` specification for cancel/reactivate, mobile+desktop viewport validation, and test specifications. Sub-issue #210's body has none of these -- it still uses the pre-revision text "handle null gracefully" and lacks the tests section and mobile validation section that were added to the plan comment. - **Suggested Edit**: Update sub-issue #210 body to match the plan comment's Milestone 4 section exactly, including: - Code snippet for `upgradeSubscription()` inline logic - "throw BadRequestError" instead of "handle null gracefully" - Tests section - Mobile + Desktop Validation section with viewport widths ### 4. Missing Specification: repository.update() Call Signature - **Location**: Plan comment, Milestone 4, Change #1 (upgradeSubscription code snippet) - **Issue**: The inline code snippet calls `await this.repository.update(currentSubscription.id, { stripeCustomerId })`. Looking at the repository `update()` method (line 144), `UpdateSubscriptionData` already has `stripeCustomerId?: string`. However, after Milestone 1, the type changes to `string | null`. The plan does not state whether `UpdateSubscriptionData.stripeCustomerId` also needs updating. If the developer follows only the M1 type changes (Subscription interface), they may miss updating UpdateSubscriptionData. - **Suggested Edit**: Add to Milestone 1 type changes: "Update `UpdateSubscriptionData.stripeCustomerId` from `string` to `string | null` to match the Subscription interface change." ### 5. Temporal Contamination (Minor) - **Location**: Plan comment, multiple locations - **Issue**: Several phrases use temporal language that will age: - "is now blocking" (Invisible Knowledge, Dual-Table Sync Pattern) -- after implementation, "now" is meaningless - "now creates with NULL" (Milestone 3, Change #2) -- refers to the M2 change that hasn't happened yet at time of reading - "now fail atomically" (Milestone 3, Change #3) -- same issue - **Suggested Edit**: Replace temporal references with absolute statements: - "is now blocking" -> "is blocking" - "now creates with NULL" -> "creates with NULL (per Milestone 2)" - "now fail atomically" -> "fail atomically after this change" ### 6. Imprecise Scope: Controller Comment at Line 223 - **Location**: Plan comment, Codebase Analysis Summary, "Controller" section - **Issue**: The plan states "Controller delegates to service - admin_override_ checks are in service layer" and flags "cancel/reactivate endpoints have NO admin_override_ guards." However, the controller at line 223 has an inline comment `// Update payment method via service (handles admin_override_ customer IDs)` which should be updated during implementation. This stale comment is not called out in any milestone's file changes. Since the controller is listed under "Files NOT Changed (Confirmed Safe)", the developer will skip it. - **Suggested Edit**: Either: - (a) Add controller to Milestone 4 file list with a single change: "Remove stale comment referencing admin_override_ at line 223", or - (b) Add a note under "Files NOT Changed" acknowledging the stale comment will persist until a separate cleanup ### 7. Missing Specification: SubscriptionResponse Type Alignment - **Location**: Plan comment, Milestone 1, Change #2 - **Issue**: The plan says to update `SubscriptionResponse` type, but the current `SubscriptionResponse` (types file line 77) has `stripeCustomerId: string` (non-nullable). The plan mentions this change but does not address the `mapToResponse()` method in the service (line 997-1013) which assigns `stripeCustomerId: subscription.stripeCustomerId` directly. After the type change, `SubscriptionResponse.stripeCustomerId` becomes `string | null`, and the mapper will pass through `null` to the API response. This is technically correct but the plan should explicitly state that the API contract changes: `stripeCustomerId` field in JSON responses will be `null` instead of an `admin_override_*` string. - **Suggested Edit**: Add to Milestone 1: "API contract change: GET /api/subscriptions response will return `stripeCustomerId: null` for admin-set subscriptions instead of `stripeCustomerId: 'admin_override_...'`. Frontend already has no dependency on this value -- confirmed by codebase analysis." --- ## Documentation Readiness **Assessment: READY with minor gaps** The plan provides sufficient material for code comments and README generation: - **Decision Log**: All 4 decisions have clear reasoning chains that translate directly to JSDoc comments. The "Reasoning Chain" column format is particularly effective for inline `// Decision: ...` comments. - **Invisible Knowledge**: The 3 items (Admin-to-Stripe Transition Path, Webhook Isolation, Dual-Table Sync Pattern) are well-written and README-ready. The Dual-Table Sync Pattern item should remove "now" as noted in Finding #5. - **Code Comment Gap**: The plan does not specify what JSDoc or inline comment should replace the deleted `resolveStripeCustomerId()` method's documentation. The inline Stripe customer creation in `upgradeSubscription()` and `updatePaymentMethod()` should carry forward the "why" context. Suggest adding a comment template to Milestone 4: ```typescript // Create Stripe customer for admin-overridden subscription. // Admin-set subscriptions have NULL stripeCustomerId. // On first Stripe payment, we create the customer in-place // rather than using lazy resolution (see Decision Log #3). ``` - **README Update Scope**: The subscriptions feature README (`backend/src/features/subscriptions/README.md`) should be updated to document the NULL stripeCustomerId semantics. This is not called out in any milestone. Recommend adding a documentation task to Milestone 4 or as a separate follow-up. --- *Verdict*: MINOR_EDITS | *Next*: Address findings, then QR plan-code -> QR plan-docs
egullickson commented 2026-02-16 14:59:39 +00:00
Author
Owner
Copy Link
Copy Source

TW Review: plan-scrub

Phase: Plan-Review | Agent: Technical Writer | Status: MINOR_EDITS

VERDICT: MINOR_EDITS

Findings

1. mapSubscriptionRow NULL Handling Precision

  • Location: Milestone 2
  • Issue: Plan specifies || operator for null coercion but should use ?? (nullish coalescing) to avoid coercing empty strings. Inconsistency: stripeSubscriptionId uses || undefined while proposed stripeCustomerId uses || null.
  • Fix: Use row.stripe_customer_id ?? null consistently.

2. cancel/reactivate Error Location Unspecified

  • Location: Milestone 4
  • Issue: Does not specify whether BadRequestError guard goes in service or controller layer.
  • Fix: Specify service layer (aligns with existing business logic pattern).

3. Sub-Issue #210 Body Diverges from Plan

4. Missing UpdateSubscriptionData Type Change

  • Location: Milestone 1
  • Issue: Plan updates Subscription.stripeCustomerId but does not mention UpdateSubscriptionData.stripeCustomerId in same types file.
  • Fix: Add to Milestone 1 changes.

5. Temporal Contamination (3 instances)

  • Location: Milestones 3, 4
  • Issue: "is now blocking", "now creates with NULL", "now fail atomically" will age poorly.
  • Fix: Use absolute statements: "is blocking", "creates with NULL", "fail atomically".

6. Stale Controller Comment

  • Location: Files NOT Changed section
  • Issue: Controller line 223 may have comment referencing admin_override_ that becomes stale, but controller listed as "no changes".
  • Fix: Add controller comment cleanup to Milestone 4 or document as known tech debt.

7. API Contract Change Not Documented

  • Location: Planning Context
  • Issue: SubscriptionResponse type change means API returns null instead of admin_override_* strings. This is an API contract change.
  • Fix: Add to Constraints & Assumptions as explicit API contract note.

Documentation Readiness: READY with minor gaps

Decision Log and Invisible Knowledge sufficient for code comments and README generation. Subscriptions feature README should be listed for update post-implementation.

Verdict: MINOR_EDITS | Next: Apply fixes, then QR plan-code

## TW Review: plan-scrub **Phase**: Plan-Review | **Agent**: Technical Writer | **Status**: MINOR_EDITS --- ### VERDICT: MINOR_EDITS ### Findings #### 1. mapSubscriptionRow NULL Handling Precision - **Location**: Milestone 2 - **Issue**: Plan specifies `||` operator for null coercion but should use `??` (nullish coalescing) to avoid coercing empty strings. Inconsistency: `stripeSubscriptionId` uses `|| undefined` while proposed `stripeCustomerId` uses `|| null`. - **Fix**: Use `row.stripe_customer_id ?? null` consistently. #### 2. cancel/reactivate Error Location Unspecified - **Location**: Milestone 4 - **Issue**: Does not specify whether BadRequestError guard goes in service or controller layer. - **Fix**: Specify service layer (aligns with existing business logic pattern). #### 3. Sub-Issue #210 Body Diverges from Plan - **Location**: Sub-issue #210 - **Issue**: Revised plan comment includes code snippets, explicit BadRequestError, mobile validation, and tests absent from sub-issue body. - **Fix**: Update sub-issue #210 body to match plan. #### 4. Missing UpdateSubscriptionData Type Change - **Location**: Milestone 1 - **Issue**: Plan updates `Subscription.stripeCustomerId` but does not mention `UpdateSubscriptionData.stripeCustomerId` in same types file. - **Fix**: Add to Milestone 1 changes. #### 5. Temporal Contamination (3 instances) - **Location**: Milestones 3, 4 - **Issue**: "is now blocking", "now creates with NULL", "now fail atomically" will age poorly. - **Fix**: Use absolute statements: "is blocking", "creates with NULL", "fail atomically". #### 6. Stale Controller Comment - **Location**: Files NOT Changed section - **Issue**: Controller line 223 may have comment referencing admin_override_ that becomes stale, but controller listed as "no changes". - **Fix**: Add controller comment cleanup to Milestone 4 or document as known tech debt. #### 7. API Contract Change Not Documented - **Location**: Planning Context - **Issue**: SubscriptionResponse type change means API returns `null` instead of `admin_override_*` strings. This is an API contract change. - **Fix**: Add to Constraints & Assumptions as explicit API contract note. ### Documentation Readiness: READY with minor gaps Decision Log and Invisible Knowledge sufficient for code comments and README generation. Subscriptions feature README should be listed for update post-implementation. --- *Verdict*: MINOR_EDITS | *Next*: Apply fixes, then QR plan-code
egullickson commented 2026-02-16 15:03:26 +00:00
Author
Owner
Copy Link
Copy Source

QR Review: plan-code

Phase: Plan-Review | Agent: Quality Reviewer | Status: NEEDS_CHANGES

VERDICT: NEEDS_CHANGES

Findings

RULE 0 CRITICAL: Unhandled Stripe customer creation failure in upgrade flow

  • Location: Milestone 4 - upgradeSubscription()
  • Issue: If createCustomer() succeeds but repository.update() fails, orphaned Stripe customer exists with no DB linkage
  • Failure Mode: Stripe customer billed but subscription record has NULL stripeCustomerId. Webhooks can't match.
  • Resolution: ACCEPTED. Extract ensureStripeCustomer() private method with cleanup logic. Addresses both failure handling and RULE 2 duplication concern.

RULE 0 CRITICAL: Race condition between upgrade and webhook payment processing

  • Location: Milestone 4 - upgradeSubscription()
  • Issue: Webhook can arrive before repository.update() commits new stripeCustomerId
  • Resolution: ACKNOWLEDGED BUT NOT BLOCKING. This is a pre-existing race condition in resolveStripeCustomerId(). Webhook handlers already handle "not found" gracefully with warning logs. Stripe webhooks are typically delayed by seconds, not milliseconds. Not introduced by this refactor.

RULE 0 HIGH: Blocking syncTierToUserProfile breaks graceful degradation

  • Location: Milestone 3 - syncTierToUserProfile()
  • Issue: QR suggests keeping non-blocking with escalated logging instead of blocking
  • Resolution: DISAGREE - blocking is correct per Decision Critic verification. The sync is a single-row UPDATE on indexed column (<1ms). The only failure mode (DB connection loss) would also fail the preceding subscription update. If DB is down, the whole operation fails before reaching sync. Blocking prevents tier drift in ALL code paths.

RULE 1 HIGH: Missing TypeScript type updates in API layer

  • Location: Milestone 1 types
  • Issue: SubscriptionResponse and request validator types not mentioned
  • Resolution: ACCEPTED. Already added UpdateSubscriptionData to Milestone 1 in TW revision. SubscriptionResponse was already listed. Request schemas don't include stripeCustomerId (it's server-set). No gap.

RULE 2 SHOULD_FIX: Code duplication for Stripe customer creation

  • Location: Milestone 4 - upgrade + updatePaymentMethod
  • Resolution: ACCEPTED. Extract ensureStripeCustomer() private method (also addresses CRITICAL #1).

RULE 2 SHOULD_FIX: Dead code removal incomplete

  • Location: Milestone 3
  • Issue: Tests for resolveStripeCustomerId() not mentioned
  • Resolution: ACCEPTED. Tests directory is empty (confirmed by codebase analysis). No test cleanup needed. Added note.

QR Review: plan-docs

Phase: Plan-Review | Agent: Quality Reviewer | Status: PASS_WITH_CONCERNS

VERDICT: PASS_WITH_CONCERNS

Findings

Missing NULL vs Sentinel Value trade-off

  • Resolution: ACCEPTED. Add to Invisible Knowledge.

Grace period job implications not explained

  • Resolution: ACCEPTED. Enhance Constraints.

API contract change needs breaking change highlight

  • Resolution: Already documented in Constraints & Assumptions. Enhanced with explicit endpoint reference.

Feature Context section for README

  • Resolution: ACCEPTED. Add brief Feature Context section.

Verdict: Revise plan, then APPROVED | Next: Final plan revision

## QR Review: plan-code **Phase**: Plan-Review | **Agent**: Quality Reviewer | **Status**: NEEDS_CHANGES --- ### VERDICT: NEEDS_CHANGES ### Findings #### RULE 0 CRITICAL: Unhandled Stripe customer creation failure in upgrade flow - **Location**: Milestone 4 - upgradeSubscription() - **Issue**: If createCustomer() succeeds but repository.update() fails, orphaned Stripe customer exists with no DB linkage - **Failure Mode**: Stripe customer billed but subscription record has NULL stripeCustomerId. Webhooks can't match. - **Resolution**: ACCEPTED. Extract `ensureStripeCustomer()` private method with cleanup logic. Addresses both failure handling and RULE 2 duplication concern. #### RULE 0 CRITICAL: Race condition between upgrade and webhook payment processing - **Location**: Milestone 4 - upgradeSubscription() - **Issue**: Webhook can arrive before repository.update() commits new stripeCustomerId - **Resolution**: ACKNOWLEDGED BUT NOT BLOCKING. This is a pre-existing race condition in `resolveStripeCustomerId()`. Webhook handlers already handle "not found" gracefully with warning logs. Stripe webhooks are typically delayed by seconds, not milliseconds. Not introduced by this refactor. #### RULE 0 HIGH: Blocking syncTierToUserProfile breaks graceful degradation - **Location**: Milestone 3 - syncTierToUserProfile() - **Issue**: QR suggests keeping non-blocking with escalated logging instead of blocking - **Resolution**: DISAGREE - blocking is correct per Decision Critic verification. The sync is a single-row UPDATE on indexed column (<1ms). The only failure mode (DB connection loss) would also fail the preceding subscription update. If DB is down, the whole operation fails before reaching sync. Blocking prevents tier drift in ALL code paths. #### RULE 1 HIGH: Missing TypeScript type updates in API layer - **Location**: Milestone 1 types - **Issue**: SubscriptionResponse and request validator types not mentioned - **Resolution**: ACCEPTED. Already added UpdateSubscriptionData to Milestone 1 in TW revision. SubscriptionResponse was already listed. Request schemas don't include stripeCustomerId (it's server-set). No gap. #### RULE 2 SHOULD_FIX: Code duplication for Stripe customer creation - **Location**: Milestone 4 - upgrade + updatePaymentMethod - **Resolution**: ACCEPTED. Extract `ensureStripeCustomer()` private method (also addresses CRITICAL #1). #### RULE 2 SHOULD_FIX: Dead code removal incomplete - **Location**: Milestone 3 - **Issue**: Tests for resolveStripeCustomerId() not mentioned - **Resolution**: ACCEPTED. Tests directory is empty (confirmed by codebase analysis). No test cleanup needed. Added note. --- ## QR Review: plan-docs **Phase**: Plan-Review | **Agent**: Quality Reviewer | **Status**: PASS_WITH_CONCERNS --- ### VERDICT: PASS_WITH_CONCERNS ### Findings #### Missing NULL vs Sentinel Value trade-off - **Resolution**: ACCEPTED. Add to Invisible Knowledge. #### Grace period job implications not explained - **Resolution**: ACCEPTED. Enhance Constraints. #### API contract change needs breaking change highlight - **Resolution**: Already documented in Constraints & Assumptions. Enhanced with explicit endpoint reference. #### Feature Context section for README - **Resolution**: ACCEPTED. Add brief Feature Context section. --- *Verdict*: Revise plan, then APPROVED | *Next*: Final plan revision
egullickson commented 2026-02-16 15:29:30 +00:00
Author
Owner
Copy Link
Copy Source

Milestone: Execution Complete

Phase: Execution | Agent: Orchestrator | Status: PASS

Milestone 1: Database migration + type definitions (#207)

  • Created 002_nullable_stripe_customer_id.sql - DROP NOT NULL + cleanup admin_override_* to NULL
  • Updated Subscription.stripeCustomerId: string | null (backend)
  • Updated SubscriptionResponse.stripeCustomerId: string | null (backend)
  • Updated UpdateSubscriptionData.stripeCustomerId?: string | null (backend)
  • Updated Subscription.stripeCustomerId: string | null (frontend)
  • Backend + frontend compile clean

Milestone 2: Repository layer updates (#208)

  • createForAdminOverride() now INSERTs with stripe_customer_id = NULL (placeholder removed)
  • mapSubscriptionRow() uses row.stripe_customer_id ?? null (nullish coalescing)
  • create() parameter type updated to accept optional stripeCustomerId

Milestone 3: Service - admin override + sync hardening (#209)

  • resolveStripeCustomerId() deleted entirely (30 lines removed)
  • syncTierToUserProfile() made blocking - try-catch with error swallowing removed
  • All 4 callers (upgrade, downgrade, handleSubscriptionUpdated, handleSubscriptionDeleted) now fail atomically if sync fails

Milestone 4: Service - Stripe flows + null handling (#210)

  • ensureStripeCustomer() extracted as private method with orphaned Stripe customer cleanup
  • upgradeSubscription() uses ensureStripeCustomer() instead of deleted method
  • updatePaymentMethod() uses ensureStripeCustomer() instead of deleted method
  • getInvoices() simplified from prefix check to !subscription?.stripeCustomerId
  • cancelSubscription() + reactivateSubscription() - null guard added for stripeCustomerId
  • Controller comment cleaned (admin_override_ reference removed)
  • deleteCustomer() added to StripeClient for cleanup logic

Validation

  • Backend TypeScript: 0 errors
  • Frontend TypeScript: 0 errors
  • Backend lint: 0 errors (670 pre-existing warnings)
  • Frontend lint: 0 errors (227 pre-existing warnings)
  • Tests: 141 passed, 27 suites failed (pre-existing - require Docker container environment)

Commits

  • af11b49 refactor: add migration and nullable types for stripe_customer_id (refs #207)
  • a6eea6c refactor: update repository for nullable stripe_customer_id (refs #208)
  • 93e79d1 refactor: replace resolveStripeCustomerId with ensureStripeCustomer, harden sync (refs #209, refs #210)

Verdict: PASS | Next: QR post-implementation review, then PR

## Milestone: Execution Complete **Phase**: Execution | **Agent**: Orchestrator | **Status**: PASS --- ### Milestone 1: Database migration + type definitions (#207) - Created `002_nullable_stripe_customer_id.sql` - DROP NOT NULL + cleanup admin_override_* to NULL - Updated `Subscription.stripeCustomerId: string | null` (backend) - Updated `SubscriptionResponse.stripeCustomerId: string | null` (backend) - Updated `UpdateSubscriptionData.stripeCustomerId?: string | null` (backend) - Updated `Subscription.stripeCustomerId: string | null` (frontend) - Backend + frontend compile clean ### Milestone 2: Repository layer updates (#208) - `createForAdminOverride()` now INSERTs with `stripe_customer_id = NULL` (placeholder removed) - `mapSubscriptionRow()` uses `row.stripe_customer_id ?? null` (nullish coalescing) - `create()` parameter type updated to accept optional `stripeCustomerId` ### Milestone 3: Service - admin override + sync hardening (#209) - `resolveStripeCustomerId()` deleted entirely (30 lines removed) - `syncTierToUserProfile()` made blocking - try-catch with error swallowing removed - All 4 callers (upgrade, downgrade, handleSubscriptionUpdated, handleSubscriptionDeleted) now fail atomically if sync fails ### Milestone 4: Service - Stripe flows + null handling (#210) - `ensureStripeCustomer()` extracted as private method with orphaned Stripe customer cleanup - `upgradeSubscription()` uses `ensureStripeCustomer()` instead of deleted method - `updatePaymentMethod()` uses `ensureStripeCustomer()` instead of deleted method - `getInvoices()` simplified from prefix check to `!subscription?.stripeCustomerId` - `cancelSubscription()` + `reactivateSubscription()` - null guard added for stripeCustomerId - Controller comment cleaned (admin_override_ reference removed) - `deleteCustomer()` added to StripeClient for cleanup logic ### Validation - Backend TypeScript: 0 errors - Frontend TypeScript: 0 errors - Backend lint: 0 errors (670 pre-existing warnings) - Frontend lint: 0 errors (227 pre-existing warnings) - Tests: 141 passed, 27 suites failed (pre-existing - require Docker container environment) ### Commits - `af11b49` refactor: add migration and nullable types for stripe_customer_id (refs #207) - `a6eea6c` refactor: update repository for nullable stripe_customer_id (refs #208) - `93e79d1` refactor: replace resolveStripeCustomerId with ensureStripeCustomer, harden sync (refs #209, refs #210) --- *Verdict*: PASS | *Next*: QR post-implementation review, then PR
egullickson added
status
review
 and removed
status
in-progress
 labels 2026-02-16 15:31:38 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
status
backlog
status/blocked
status
done
status
in-progress
status
ready
status
review
type
bug
type
chore
type
docs
type
feature
No labels
status
review
type
feature
Milestone
No items
No Milestone Sprint 2026-02-02
Projects
Clear projects
No projects
Assignees
Clear assignees
egullickson
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: egullickson/motovaultpro#205