pal-e-docs

Review: #497 Recover stranded monthly-fee parents (analog of #486)

review-1033-2026-04-17 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Scope is directionally sound and the ticket accurately mirrors #486's three-phase structure. However, the nuclear gate collapses 6 steps into a single checkbox — #486's "HARD STOP" banner and per-gate checkboxes set the floor, and #497 falls short of that floor. Phase C observability also bloats the ticket beyond the recovery scope.

Template Completeness

  • [x] Type (Task)
  • [x] Lineage (references #486, 2026-04-17 date, stranded $460 evidence)
  • [x] Repo (forgejo_admin/basketball-api)
  • [x] User Story (matches story:WS-S22)
  • [x] Context (Stripe mechanism, stranded table with 5 orders, historical canceled investigation rationale)
  • [x] Scope (three phases labeled)
  • [x] Acceptance Criteria
  • [x] Constraints
  • [x] Related (links #486, #494, #493, memory note)
  • [ ] HARD STOP banner — MISSING. #486 opens with a ⚠️ HARD STOP — NO EMAILS WITHOUT APPROVAL ⚠️ section that enumerates the 6 gates and states "Approved draft ≠ approved to send. One approval = one send." This is the single most important template convention for blast tickets and #497 has no equivalent.
  • [ ] Checklist section — MISSING. #486 has a Checklist enumerating dry-run, template PR, gates, blast, payment confirmation, runbook. #497 folds everything into AC.

Traceability

  • [x] story:WS-S22 label — "clear email communications with action links so that I never miss a deadline"
  • [x] story note verified — found in project-westside-basketball user-stories section (stories-parent)
  • [x] arch:dataflow-westside-basketball label
  • [x] arch note verified — arch-dataflow-westside-basketball exists (search confirmed)
  • [x] Forgejo issue — #497, open, body well-formed

File Targets

Not applicable — Task type. Scope references the monthly-fee mint path generically. Live repo confirms:
  • src/basketball_api/routes/checkout.py — prorated first-month path at _calculate_prorated_cents (line 293), uses stripe.checkout.Session.create with no expires_at override. Confirms 24h cap claim.
  • src/basketball_api/routes/admin.py — admin regen path (line 2005) — same pattern, no TTL override.
  • No monthly_checkout.py helper exists — monthly flow has no dedicated service module (unlike tournament, which has services/tournament_checkout.py). #498 will need to create one.
  • No CHECKOUT_SESSION_TTL_SECONDS constant present — good, consistent with #493 revert.

Repo Placement

OK — forgejo_admin/basketball-api. Recovery data access + email blast both live in that repo. No cross-repo spillover expected.

Dependencies

  • #486 (tournament recovery) — precedent pattern, already executed. Reuse nuclear-gate structure.
  • #498 (monthly Payment Links migration) — soft dependency. Ticket states correctly: "Blocked on the sibling architecture migration ticket IF we decide monthly-recovery should mint Payment Links (non-expiring) — OR we accept 24h TTL." This matches Ava's framing: can use helper if it ships first, but can fall back to Sessions with tight turnaround.
  • #494 (tournament Payment Links helper) — not directly consumed, but is the architectural precedent #498 copies from. Reference is accurate.
  • #493 (revert of 30-day TTL approach) — context anchor. Tells dev agent not to reintroduce the TTL constant. Accurate.
  • No blocker currently in in_progress column that would stall Phase A.

Acceptance Criteria

Phase A (3 AC) — testable. Dry-run recipient list to Lucas before drafting is explicit. Roster/subscription cross-reference explicit. OK.
Phase B — weakened gate. A single bullet — "all 6 nuclear-gate steps logged" — replaces #486's six individual gate checkboxes plus the HARD STOP banner. This is the biggest refinement need. Per feedback_email_blast_nuclear_gate.md, the gate must be reproduced verbatim, not referenced. A single checkbox invites a dev agent to mark it complete after a perfunctory approval chain.
Phase C — 1 AC ("mechanism exists for Lucas to request a paid/unpaid snapshot on demand, at minimum"). This is vague, unbounded, and not scoped like a recovery ticket should be. "At minimum" is an escape hatch that can swallow arbitrary observability work.

Blast Radius

  • Related blast risk: Same class of customer-facing incident as #486. If Phase B emails go out under the weakened gate, a 2026-04-04 or 2026-04-13 style incident could repeat. This is the exact risk the nuclear-gate feedback was created to prevent.
  • Test parent exclusion: Ticket notes "Exclude test parent rows (admin / test seed data) — same principle as order 93 exclusion in #486." Acknowledged but not verified — the dev agent needs to explicitly grep the 5 stranded parent rows for admin/test accounts before Phase A dry-run goes to Lucas.
  • Per-parent variable amount: players.monthly_fee or orders.amount_cents used as source. Ticket constraint correctly flags: each Payment Link must charge the exact amount_cents on that specific Order. Accurate — unlike tournament's uniform per-product rate.
  • Silent-abandon parents in canceled state: Ticket says parents 166 and 170 may be true silent abandons (canceleds + still-pending without paid record). Phase A must resolve these explicitly; currently a single AC ("exhaustive list") covers the investigation but does not require per-parent classification (paid-via-retry / normal-pending / true-stranded). Adding a classification requirement would sharpen Phase A deliverable.

Decomposition Assessment

NEEDS DECOMPOSITION. Three distinct phases, three distinct exit criteria, three distinct risk profiles:
  • Phase A (investigation + dry-run list) — 1 agent, ~15-30 min. Pure data / SQL + Stripe retrieve work. No customer risk.
  • Phase B (recovery blast) — cannot start until Phase A is Lucas-approved. Must be a separate ticket with its own HARD STOP banner, its own 7-step gate reproduced verbatim, its own single-responsibility scope. Coupling A + B in one ticket lets a dev agent "finish" A and drift into drafting emails.
  • Phase C (observability) — orthogonal to recovery. Should be a standalone Feature ticket that survives independent of whether the blast happens. Currently bloats the ticket.
Recommended split: keep #497 as Phase A only. New ticket for Phase B (gated by Phase A approval). New ticket for Phase C (Feature, not Task).

Recommendation

  • [BODY] Add a ⚠️ HARD STOP — NO EMAILS WITHOUT APPROVAL ⚠️ banner at the top of the issue body, identical in structure to #486's. Enumerate the 6 gates verbatim. Include "Approved draft ≠ approved to send. One approval = one send." State test recipients are draneylucas@gmail.com or @example.com only.
  • [BODY] Replace the single Phase B checkbox "all 6 nuclear-gate steps logged" with the 6 individual checkboxes (Lucas approves draft / Marcus approves draft / test email sent / Lucas confirms test / Lucas says BLAST / Ava reads back / Lucas says BLAST #2). Match #486's Phase A block verbatim.
  • [BODY] Add explicit Constraint: "No email drafting, MJML work, or blast endpoint invocation may occur during Phase A. Phase A output = recipient list + classification only."
  • [BODY] Strengthen Phase A AC: require per-parent classification (paid-via-retry / normal-pending / true-stranded / test-account-excluded) rather than just "exhaustive list." This is the deliverable.
  • [DECOMPOSE] Split the ticket: keep #497 as Phase A (investigation + dry-run recipient list). Open a new Forgejo issue for Phase B (recovery blast, blocked on #497 Phase A completion + Lucas approval). Open a new Forgejo issue for Phase C (paid/unpaid observability, standalone Feature, decoupled from recovery). Route to skill-decompose-ticket.
  • [BODY] Replace the vague Phase C AC ("mechanism exists... at minimum") with a concrete scope once lifted into its own ticket — e.g., a single SQL view + admin endpoint, or a weekly cron report to westsidebasktball@gmail.com.
  • [BODY] Add a Checklist section mirroring #486's (dry-run shared, gates completed in order, blast sent, payment confirmed, docs updated).