fix(learning-engine): league rollover catch-up, local-day streak nudge, FSRS lapsed backfill, interleaving gate, store races (#549) - #559
Merged
Conversation
…e, FSRS lapsed backfill, interleaving gate, store races (#549) Six correctness defects across the shipped learning-engine work (#389 FSRS, #393 interleaving, #394 leagues + store, #397 digest), none of which had test coverage, plus the test runner four of them needed. Stand up Vitest in mcp-server/ (no runner existed at all) and characterize gradeCard, cardFromRow, computeInterleavingPool, eloExpected and eloJitter against current behaviour before changing anything. §1 A single missed weekly rollover wiped every student's tier. The previous week was hardcoded as v_week - 7, so one skipped Monday tick left that join matching nothing and COALESCE(..., 1) reset the whole tenant to Bronze, while the genuinely-previous week kept final_rank NULL forever. The previous week is now resolved from the data and every unfinalized week is finalized, each over its own [w, w+7) XP window — carrying one window across a multi-week gap would rank a stale cohort over unrelated activity. §2 Promotion/relegation carried no information at the minimum cohort size: promote_count = demote_count = 5 against a cold-start floor of 10 meant the bands exactly tiled the cohort and 'stayed' was unreachable. Bands now scale with active cohort size and always leave at least one 'stayed'. Zero-XP members are excluded from that size and parked, so an inactive student can no longer promote over an active one on a UUID tiebreak. §3 The streak nudge was inverted for the LATAM market. isStreakAtRisk compared against the UTC day while the send decision and idempotency key are tenant-local, so at UTC-5 a student who practised that local afternoon was told their streak was ending hours after securing it, and the students who actually skipped got nothing. Comparison is now against the tenant-local day boundary, computed calendar-first so it survives DST. award_xp's own day boundary is deliberately unchanged — moving it would shift existing streaks — and the residual is documented at the call site. §4 The FSRS backfill skipped the hardest cards. Its predicate was repetitions > 0, but the SM-2 code it replaced reset repetitions to 0 on a lapse, so every card whose last rating was "Again" was excluded and then silently downgraded to createEmptyCard(). Widened to include lapsed rows, with difficulty derived from the retained ease column and state 3 (Relearning). §5 The interleaving gate was advisory. lms_record_practice_attempt accepted mode='mixed' with no gate and no kill-switch check, and its own description told the model to call it directly, so the bypass was the documented path — and the rows it wrote were the rows the gate later read back, letting a session certify its own topics. Both tools now share one gate decision. Separately, XP is awarded once per session rather than once per stored row, so a 3-topic mixed session no longer pays 45 XP and burns 3 of the 10 daily slots against 15 XP for the same work done focused. §6 spend-points double-spent coins and over-granted freezes via three unsynchronized read-modify-writes against a derived balance. Replaced with a single redeem_store_item() RPC holding SELECT ... FOR UPDATE on the profile row, which also refuses at the 5-freeze ceiling instead of charging for a grant that cannot land. Verified: 467/467 root unit tests, 35 mcp-server tests, typecheck and build clean, and seeded-DB assertions for §1/§2/§4/§5/§6 committed under tests/sql/issue-549/ — including a real 5-way concurrency race proving the store redemptions serialize. Closes #549 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RGCEptT7pgWwRiMdB3TUCM
guillermoscript
marked this pull request as ready for review
July 26, 2026 17:50
Merged
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the six correctness defects in issue #549 across the shipped learning-engine work (#389 FSRS, #393 interleaving, #394 leagues + store, #397 digest), and stands up the Vitest runner in
mcp-server/that four of them needed but did not have.Closes #549
Why
Each of the five features landed with a correct happy path and an unexamined boundary. None had test coverage, and
mcp-server/had no test runner at all, so step 0 was standing one up and characterizing the affected pure functions against current behaviour before changing anything.§1 — one skipped league rollover wiped every student's tier. The previous week was hardcoded as
v_week - 7. Miss a single Monday 00:05 tick and that join matches zero rows,COALESCE(…, 1)resets the whole tenant to Bronze, and the genuinely-previous week keepsfinal_rank/movementNULL forever. The previous week is now resolved from the data (greatestweek_startbelow the target that actually has rows), and every unfinalized older week is finalized, each over its own[w, w + 7)XP window. That window is the load-bearing half: carrying one window across a multi-week gap would rank a stale cohort over weeks of unrelated activity and produce confidently wrong ranks.This also makes the ordinary weekly run its own catch-up path, so the cron route needs no
_week_startargument — only a doc comment, which it got.§2 — promotion/relegation carried no information at the minimum cohort size. All five tiers seed
promote_count = demote_count = 5against a cold-start floor of 10, so at exactly 10 the bands tile the cohort and'stayed'is unreachable — the tier a student holds says nothing. Bands now scale:LEAST(tier's count, GREATEST(1, active_size / 5), (active_size - 1) / 2). The third term guarantees at least one'stayed'for any cohort of 3+. Zero-XP members are excluded from the size the bands are computed against and parked as'stayed', so an inactive student can no longer promote over an active one on a membership-UUID tiebreak.§3 — the streak nudge was inverted for the LATAM market.
isStreakAtRiskcomparedlast_activity_dateagainst UTC yesterday while the send decision (localHour) and the idempotency key (localDateStr) are tenant-local. At UTC−5 the 20:00 nudge runs at 01:00 UTC the next UTC day, so a student who practised that same local afternoon was told their streak was ending hours after they had secured it, while the students who actually skipped the local day failed the equality and got nothing. Fires whenevernudgeHour + |utc_offset| >= 24. Now compared against the tenant-local day boundary, computed calendar-first so it is correct across DST. The candidate RPC's matching= CURRENT_DATE - 1pre-filter widened to>= CURRENT_DATE - 2so it is a genuine superset for every offset and the precise call happens where the timezone is known.§4 — the FSRS backfill skipped the hardest cards. The predicate was
repetitions > 0, but the SM-2 code it replaced resetrepetitionsto 0 on a lapse. So it selected exactly the cards that were not struggling and skipped every card whose last rating was "Again", whichcardFromRowthen silently downgraded tocreateEmptyCard(). Widened torepetitions > 0 OR last_reviewed_at IS NOT NULL, with difficulty derived from the retainedeasecolumn. Thestability IS NULLguard keeps it re-runnable and leaves already-seeded rows untouched.§5 — the interleaving gate was advisory, and mixed sessions multiplied XP.
lms_record_practice_attemptacceptedmode: 'mixed'with arbitrary topics, no gate and no kill-switch check — and its own description instructs the model to call it directly for free-text and chat/voice drills, so the bypass was the documented path. Worse, the rows it wrote are the rowsfetchInterleavingPoolreads back, so an ungated session certified its own topics. Both tools now route through one sharedblockedInterleavingTopics()decision, evaluated before the insert. Separately, XP moved from once-per-row to once-per-session, so a 3-topic session no longer pays 45 XP and burns 3 of the 10 daily slots against 15 XP for the same work done focused.§6 —
spend-pointsdouble-spent coins and over-granted freezes. Three unsynchronized read-modify-writes against a derived balance (floor(total_xp / 10) − total_coins_spent), so a lost update meant the student kept the item and the coins. Replaced with a singleredeem_store_item()RPC holdingSELECT … FOR UPDATEon the profile row for the balance check, cap check, redemption insert, coin debit and reward grant. It also now refuses at the 5-freeze ceiling instead of charging for a grant that cannot land.Two deliberate deviations from the issue's proposed fixes
streak_freezeis seeded withmax_per_user = 5, not 1 (20260216000001_seed_gamification_data.sql:63), and a unique index cannot express "at most 5 per calendar month" — adding one would silently cap the item at 1/month and change product behaviour. The profile row lock already serializes every redemption for a given user, which is what makes the surviving count-then-insert safe. That is the mechanism relied on here.award_xp's day boundary is unchanged (§3), as the issue directs. That leaves one residual: a student whose only activity fell in the local evening after the UTC day rolled over is still measured against the UTC day. It is documented at the call site rather than left looking fixed.How to QA
Backend-only — nothing user-visible changed, so there are no screenshots. Everything below runs on a fresh
npm run db:reset.Automated:
npm run test:unit→ 467 passing. The LATAM regression istests/unit/daily-digest.test.ts→ "LATAM regression (issue Learning engine correctness: leagues, streak nudge timezone, FSRS backfill, interleaving gate, store races #549 §3)", which asserts a Bogotá student who practised that local day is not nudged and one who skipped is. The old code got both backwards.cd mcp-server && npm test→ 35 passing. This runner is new; there was none before.npm run typecheckandnpm run build→ clean.Seeded-DB assertions (the parts that live in SQL and no Vitest suite can reach) are committed under
tests/sql/issue-549/with a runbook. Each script rolls itself back and raises an exception on failure rather than printing rows to eyeball:test-leagues-549.sql— seeds 12 students, skips a week, then rolls over. Asserts tiers are carried (2 Bronze — the demoted pair — not 12), the skipped week is finalized, and at least one'stayed'exists. It also plants a 999999-XP award outside the ranked week for the last-placed student: if the XP window ever leaks across weeks again, that student ranks first and the assertion fails.test-fsrs-549.sql— a lapsed card gets non-NULLstability/difficultyderived from itsease; never-reviewed and already-seeded cards are untouched; the original predicate is shown matching zero rows against the same lapsed card.test-practice-xp-549.sql— a 3-topic mixed session produces exactly one 15-XP transaction; adding a focused session takes it to two / 30 XP.test-store-race-549.sql— sequential correctness, plus the ceiling refusing without changingtotal_coins_spent.psqlprocesses) has its own setup/verify/cleanup trio and a copy-pasteable driver intests/sql/issue-549/README.md. Observed: all 5 return distinctcoins_remaining(4000 / 3000 / 2000 / 1000 / 0 — they serialized),total_coins_spentexactly 5000, 5 redemptions, no lost update.Reproducing the §1 bug, if you want to see it fail first: restore the pre-fix
rollover_leaguesbody (lines 235–400 of20260717120000_create_weekly_leagues.sql) inside a transaction and run the same league scenario. All 12 students collapse to Bronze and week W stays entirely unfinalized. That is how each of these was confirmed before being fixed.Screenshots / GIF
N/A — no user-visible surface changed. The changes are five migrations, one edge function, one MCP tool, and one notification helper.
Checklist
npm run typecheckandnpm run test:unitpassnpm run buildpassestenant_id(or the table genuinely has no such column)spend-pointsresponse shape is preserved, including theavailable/requiredfields oninsufficient_coins, so existing clients keep working; new refusals carry a machine-readablecodemessages/en.jsonandmessages/es.json— N/A, no UI stringslib/database.types.tswas regeneratedOn roles: these paths are not role-branched — the rollover and digest run as
service_rolefrom cron,redeem_store_itemisservice_role-only with the edge function authenticating the caller, and the practice tools are self-scoped under the caller's own RLS client. The role-relevant assertion is that none of the new SQL is reachable byanon/authenticated; all three new/changed functions are explicitlyREVOKEd from both.On
lib/database.types.ts: updated surgically (thepractice_attempts.session_idcolumn and theredeem_store_itemsignature) rather than by regenerating the whole file. A full regeneration also sweeps in unrelated drift from other recently merged migrations (platform_payment_requests.expires_at,tenants.plan_override_*,can_read_exam) and drops the__InternalSupabaseversion block, which would bury this diff in churn that isn't mine. Those additions were verified byte-identical to whatsupabase gen types --localproduces.Risk
stability IS NULL, so it can only touch cards that are currently being mistreated.