diff --git a/app/api/cron/league-rollover/route.ts b/app/api/cron/league-rollover/route.ts index 0995a58f..cfc9f294 100644 --- a/app/api/cron/league-rollover/route.ts +++ b/app/api/cron/league-rollover/route.ts @@ -6,6 +6,11 @@ * all handled in SQL. pg_cron ('league-weekly-rollover', Mondays 00:05) is the * primary scheduler — this route is a manual trigger + Dokploy/Vercel fallback. * + * Missed ticks need no special handling and no _week_start argument: since + * issue #549 the rollover resolves the previous week from the data and + * finalizes every still-unfinalized week, so simply running it late is the + * catch-up path. It used to reset every student to Bronze in that situation. + * * Secured by CRON_SECRET (Bearer token). */ diff --git a/lib/database.types.ts b/lib/database.types.ts index 507be5dd..edf11896 100644 --- a/lib/database.types.ts +++ b/lib/database.types.ts @@ -4793,6 +4793,7 @@ export type Database = { mode: string questions: Json score: number + session_id: string source: string source_exercise_id: number | null tenant_id: string @@ -4810,6 +4811,7 @@ export type Database = { mode?: string questions: Json score: number + session_id?: string source?: string source_exercise_id?: number | null tenant_id: string @@ -4827,6 +4829,7 @@ export type Database = { mode?: string questions?: Json score?: number + session_id?: string source?: string source_exercise_id?: number | null tenant_id?: string @@ -6517,6 +6520,10 @@ export type Database = { Returns: undefined } publish_scheduled_lessons: { Args: never; Returns: undefined } + redeem_store_item: { + Args: { _item_id: string; _tenant_id: string; _user_id: string } + Returns: Json + } refresh_leaderboard_cache: { Args: never; Returns: undefined } register_push_token: { Args: { _device_name?: string; _platform: string; _token: string } diff --git a/lib/notifications/daily-digest.ts b/lib/notifications/daily-digest.ts index aaae1f26..b1c07218 100644 --- a/lib/notifications/daily-digest.ts +++ b/lib/notifications/daily-digest.ts @@ -107,15 +107,45 @@ export function localDateStr(now: Date, timezone: string): string { } } -/** UTC yesterday as YYYY-MM-DD — matches award_xp()'s CURRENT_DATE streak math. */ -export function utcYesterday(now: Date): string { - const d = new Date(now.getTime() - 24 * 60 * 60 * 1000) - return d.toISOString().slice(0, 10) +/** + * YYYY-MM-DD for the day before `now` in the given IANA timezone. + * + * Takes the local calendar date first, then steps back one calendar day on that + * date alone. Subtracting 24h from the instant would land on the wrong date + * across a DST transition. + */ +export function localYesterday(now: Date, timezone: string): string { + const [y, m, d] = localDateStr(now, timezone).split('-').map(Number) + return new Date(Date.UTC(y, m - 1, d - 1)).toISOString().slice(0, 10) } -/** Streak survives only if the student acts today: last activity was exactly yesterday. */ -export function isStreakAtRisk(lastActivityDate: string | null, streak: number, now: Date, min: number): boolean { - return streak >= min && lastActivityDate === utcYesterday(now) +/** + * Streak survives only if the student acts today: last activity was exactly + * yesterday *in the tenant's timezone*. + * + * This compared against UTC yesterday until issue #549. Everything else about + * the send is tenant-local — `localHour` decides whether to send at all, and + * `localDateStr` keys idempotency — so at UTC-5 (Bogotá, Lima) the 20:00 nudge + * runs at 01:00 UTC the *next* UTC day, and 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 were silently + * skipped. It misfired whenever `nudgeHour + |utc_offset| >= 24`. + * + * Known residual: `last_activity_date` is still written by award_xp() from + * CURRENT_DATE, i.e. the UTC day, so a student whose only activity fell in the + * local evening after the UTC day rolled over is still measured against the + * wrong day. Fixing that means changing award_xp()'s day boundary, which would + * shift every existing streak — a deliberate migration, not a side effect of + * this one (issue #549 §3). + */ +export function isStreakAtRisk( + lastActivityDate: string | null, + streak: number, + now: Date, + min: number, + timezone: string +): boolean { + return streak >= min && lastActivityDate === localYesterday(now, timezone) } const SUMMARY_COPY = { @@ -290,8 +320,8 @@ export async function runDailyDigest(admin: SupabaseClient, now: Date = new Date kind === 'daily_digest' ? c.due_cards > 0 || c.goals_pending > 0 || - isStreakAtRisk(c.last_activity_date, c.current_streak, now, DIGEST_STREAK_MIN) - : isStreakAtRisk(c.last_activity_date, c.current_streak, now, NUDGE_STREAK_MIN) + isStreakAtRisk(c.last_activity_date, c.current_streak, now, DIGEST_STREAK_MIN, settings.timezone) + : isStreakAtRisk(c.last_activity_date, c.current_streak, now, NUDGE_STREAK_MIN, settings.timezone) ) if (recipients.length === 0) continue @@ -321,7 +351,8 @@ export async function runDailyDigest(admin: SupabaseClient, now: Date = new Date candidate.last_activity_date, candidate.current_streak, now, - kind === 'daily_digest' ? DIGEST_STREAK_MIN : NUDGE_STREAK_MIN + kind === 'daily_digest' ? DIGEST_STREAK_MIN : NUDGE_STREAK_MIN, + settings.timezone ) const summary = buildSummary( { diff --git a/mcp-server/package.json b/mcp-server/package.json index e0a8f68e..f41597b6 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -25,6 +25,8 @@ "dev": "mcp-use dev", "start": "mcp-use start", "deploy": "mcp-use deploy", + "test": "vitest run", + "test:watch": "vitest", "postinstall": "mcp-use generate-types || exit 0" }, "dependencies": { diff --git a/mcp-server/src/tools/flashcards.test.ts b/mcp-server/src/tools/flashcards.test.ts new file mode 100644 index 00000000..0092b3be --- /dev/null +++ b/mcp-server/src/tools/flashcards.test.ts @@ -0,0 +1,179 @@ +import { describe, it, expect } from "vitest"; +import { gradeCard, cardFromRow } from "./flashcards.js"; + +/** + * Characterization tests (issue #549) — these capture current behaviour of + * `cardFromRow`/`gradeCard` in mcp-server/src/tools/flashcards.ts so a later + * refactor shows up as a visible test diff. Do NOT change source behaviour + * to make these pass; if source is wrong, the test documents the bug. + */ + +const NOW = new Date("2026-07-26T12:00:00.000Z"); + +type FsrsRow = Parameters[0]; + +/** A row that has never been through FSRS — the SM-2/pre-migration shape. */ +function freshRow(overrides: Partial = {}): FsrsRow { + return { + interval_days: 0, + repetitions: 0, + due_at: NOW.toISOString(), + last_reviewed_at: null, + stability: null, + difficulty: null, + fsrs_state: 0, + lapses: 0, + learning_steps: 0, + elapsed_days: 0, + ...overrides, + }; +} + +/** A fully FSRS-populated row (as a migration-seeded / previously-graded row would be). */ +function populatedRow(overrides: Partial = {}): FsrsRow { + return { + interval_days: 6, + repetitions: 3, + due_at: "2026-07-30T00:00:00.000Z", + last_reviewed_at: "2026-07-24T00:00:00.000Z", + stability: 12.34, + difficulty: 5.67, + fsrs_state: 2, // Review + lapses: 1, + learning_steps: 0, + elapsed_days: 6, + ...overrides, + }; +} + +describe("cardFromRow", () => { + it("returns a fresh empty card when stability is null", () => { + const row = freshRow({ stability: null, difficulty: 5 }); + const card = cardFromRow(row, NOW); + expect(card.state).toBe(0); // State.New + expect(card.stability).toBe(0); + expect(card.difficulty).toBe(0); + expect(card.reps).toBe(0); + expect(card.lapses).toBe(0); + expect(card.due).toEqual(NOW); + expect(card.last_review).toBeUndefined(); + }); + + it("returns a fresh empty card when difficulty is null", () => { + const row = freshRow({ stability: 5, difficulty: null }); + const card = cardFromRow(row, NOW); + expect(card.state).toBe(0); // State.New + expect(card.stability).toBe(0); + expect(card.difficulty).toBe(0); + expect(card.due).toEqual(NOW); + expect(card.last_review).toBeUndefined(); + }); + + it("returns a fresh empty card when both stability and difficulty are null", () => { + const row = freshRow(); + const card = cardFromRow(row, NOW); + expect(card.state).toBe(0); + expect(card.due).toEqual(NOW); + }); + + it("round-trips a fully populated row onto the Card", () => { + const row = populatedRow(); + const card = cardFromRow(row, NOW); + expect(card.stability).toBe(row.stability); + expect(card.difficulty).toBe(row.difficulty); + expect(card.reps).toBe(row.repetitions); + expect(card.lapses).toBe(row.lapses); + expect(card.state).toBe(row.fsrs_state); + expect(card.scheduled_days).toBe(row.interval_days); + expect(card.elapsed_days).toBe(row.elapsed_days); + expect(card.learning_steps).toBe(row.learning_steps); + expect(card.due).toEqual(new Date(row.due_at)); + expect(card.last_review).toEqual(new Date(row.last_reviewed_at as string)); + }); + + it("falls back to `now` for last_review when last_reviewed_at is null", () => { + const row = populatedRow({ last_reviewed_at: null }); + const card = cardFromRow(row, NOW); + expect(card.last_review).toEqual(NOW); + }); +}); + +describe("gradeCard", () => { + const ratings = ["again", "hard", "good", "easy"] as const; + + it.each(ratings)("returns the full persisted-row shape for rating '%s' on a New card", (rating) => { + const row = freshRow(); + const result = gradeCard(row, rating, NOW); + + expect(result).toMatchObject({ + stability: expect.any(Number), + difficulty: expect.any(Number), + fsrs_state: expect.any(Number), + lapses: expect.any(Number), + learning_steps: expect.any(Number), + elapsed_days: expect.any(Number), + interval_days: expect.any(Number), + repetitions: expect.any(Number), + due_at: expect.any(String), + last_reviewed_at: expect.any(String), + }); + + // due_at / last_reviewed_at are ISO strings. + expect(new Date(result.due_at).toISOString()).toBe(result.due_at); + expect(new Date(result.last_reviewed_at).toISOString()).toBe(result.last_reviewed_at); + expect(result.last_reviewed_at).toBe(NOW.toISOString()); + }); + + it("orders scheduling further out as rating improves: again < hard < good < easy", () => { + const row = freshRow(); + const again = gradeCard(row, "again", NOW); + const hard = gradeCard(row, "hard", NOW); + const good = gradeCard(row, "good", NOW); + const easy = gradeCard(row, "easy", NOW); + + const t = (r: ReturnType) => new Date(r.due_at).getTime(); + + expect(t(again)).toBeLessThanOrEqual(t(hard)); + expect(t(hard)).toBeLessThanOrEqual(t(good)); + expect(t(good)).toBeLessThan(t(easy)); + }); + + it("rating 'again' on an established Review-state card increments lapses", () => { + const row = populatedRow({ fsrs_state: 2, lapses: 1 }); + const result = gradeCard(row, "again", NOW); + expect(result.lapses).toBe(2); + }); + + it("BUG (#549 §4): a 'lapsed' row (repetitions/interval reset but stability/difficulty null) currently grades as a brand-new card, discarding history", () => { + // This row represents history that was reset to null stability/difficulty + // (e.g. a lapse-tracking path) while repetitions/interval_days/last_reviewed_at + // still carry prior state. Because cardFromRow() only checks + // stability===null || difficulty===null, this row is treated as a + // never-seen card: FSRS starts a brand-new learning card, and prior + // repetitions/interval history is silently discarded. Asserting today's + // (buggy) behaviour here — do NOT "fix" the source to make this pass. + const lapsedRow = freshRow({ + repetitions: 0, + interval_days: 0, + last_reviewed_at: "2026-07-20T00:00:00.000Z", + stability: null, + difficulty: null, + }); + + const card = cardFromRow(lapsedRow, NOW); + // Discards history: comes back as a brand-new createEmptyCard(now) — the + // null-stability/difficulty check short-circuits BEFORE the + // last_reviewed_at fallback logic even runs, so last_review is left + // `undefined` (createEmptyCard's default), not `now` and not the row's + // real last_reviewed_at. + expect(card.state).toBe(0); // State.New + expect(card.reps).toBe(0); + expect(card.last_review).toBeUndefined(); + + const result = gradeCard(lapsedRow, "good", NOW); + // A brand-new "good" grade produces the same result whether or not the + // row had prior repetitions — because cardFromRow() throws that history away. + const freshResult = gradeCard(freshRow(), "good", NOW); + expect(result).toEqual(freshResult); + }); +}); diff --git a/mcp-server/src/tools/practice.test.ts b/mcp-server/src/tools/practice.test.ts new file mode 100644 index 00000000..34dab19d --- /dev/null +++ b/mcp-server/src/tools/practice.test.ts @@ -0,0 +1,215 @@ +import { describe, it, expect } from "vitest"; +import { + blockedInterleavingTopics, + computeInterleavingPool, + eloExpected, + eloJitter, +} from "./practice.js"; + +/** + * Characterization tests (issue #549) — capture current behaviour of + * `computeInterleavingPool`, `eloExpected`, and `eloJitter` in + * mcp-server/src/tools/practice.ts so a later refactor shows up as a visible + * test diff. Do NOT change source behaviour to make these pass. + */ + +const DAY_MS = 24 * 60 * 60 * 1000; + +/** ISO timestamp `daysAgo` days before now (relative, so tests don't rot). */ +function daysAgo(days: number): string { + return new Date(Date.now() - days * DAY_MS).toISOString(); +} + +describe("computeInterleavingPool", () => { + it("marks a topic ready once it has >= 2 passing (>=70) attempts within 90 days", () => { + const rows = [ + { topic: "algebra", score: 80, created_at: daysAgo(1) }, + { topic: "algebra", score: 75, created_at: daysAgo(2) }, + ]; + const pool = computeInterleavingPool(rows); + const stat = pool.get("algebra"); + expect(stat).toBeDefined(); + expect(stat!.ready).toBe(true); + expect(stat!.passing_attempts).toBe(2); + expect(stat!.attempts_counted).toBe(2); + }); + + it("does not mark a topic ready with only 1 passing attempt", () => { + const rows = [{ topic: "algebra", score: 80, created_at: daysAgo(1) }]; + const pool = computeInterleavingPool(rows); + const stat = pool.get("algebra")!; + expect(stat.ready).toBe(false); + expect(stat.passing_attempts).toBe(1); + expect(stat.attempts_counted).toBe(1); + }); + + it("is present but not ready when 2 attempts both score below 70, with correct counts", () => { + const rows = [ + { topic: "algebra", score: 50, created_at: daysAgo(1) }, + { topic: "algebra", score: 60, created_at: daysAgo(2) }, + ]; + const pool = computeInterleavingPool(rows); + const stat = pool.get("algebra")!; + expect(stat.ready).toBe(false); + expect(stat.passing_attempts).toBe(0); + expect(stat.attempts_counted).toBe(2); + expect(stat.avg_score).toBe(55); + }); + + it("excludes rows older than the 90-day window entirely — a topic with only stale rows is absent", () => { + const rows = [ + { topic: "geometry", score: 90, created_at: daysAgo(91) }, + { topic: "geometry", score: 95, created_at: daysAgo(120) }, + ]; + const pool = computeInterleavingPool(rows); + expect(pool.has("geometry")).toBe(false); + expect(pool.size).toBe(0); + }); + + it("rounds avg_score and averages over ALL in-window attempts, not just passing ones", () => { + const rows = [ + { topic: "calculus", score: 100, created_at: daysAgo(1) }, // passing + { topic: "calculus", score: 90, created_at: daysAgo(2) }, // passing + { topic: "calculus", score: 40, created_at: daysAgo(3) }, // failing, still counted in avg + ]; + const pool = computeInterleavingPool(rows); + const stat = pool.get("calculus")!; + expect(stat.attempts_counted).toBe(3); + expect(stat.passing_attempts).toBe(2); + // (100 + 90 + 40) / 3 = 76.666... -> rounds to 77 + expect(stat.avg_score).toBe(77); + expect(stat.ready).toBe(true); + }); + + it("handles scores arriving as strings (Number(row.score) coercion)", () => { + const rows = [ + { topic: "physics", score: "80" as unknown as number, created_at: daysAgo(1) }, + { topic: "physics", score: "75" as unknown as number, created_at: daysAgo(2) }, + ]; + const pool = computeInterleavingPool(rows); + const stat = pool.get("physics")!; + expect(stat.passing_attempts).toBe(2); + expect(stat.ready).toBe(true); + expect(stat.avg_score).toBe(78); // (80+75)/2 = 77.5 -> rounds to 78 + }); + + it("returns an empty Map for no rows", () => { + const pool = computeInterleavingPool([]); + expect(pool.size).toBe(0); + }); + + it("tracks multiple topics independently", () => { + const rows = [ + { topic: "algebra", score: 90, created_at: daysAgo(1) }, + { topic: "algebra", score: 90, created_at: daysAgo(2) }, + { topic: "geometry", score: 30, created_at: daysAgo(1) }, + ]; + const pool = computeInterleavingPool(rows); + expect(pool.size).toBe(2); + expect(pool.get("algebra")!.ready).toBe(true); + expect(pool.get("geometry")!.ready).toBe(false); + }); +}); + +describe("eloExpected", () => { + it("returns exactly 0.5 for equal ratings", () => { + expect(eloExpected(1500, 1500)).toBe(0.5); + }); + + it("returns ~0.909 for a 400-point student advantage", () => { + expect(eloExpected(1900, 1500)).toBeCloseTo(0.9091, 3); + }); + + it("returns ~0.0909 for a 400-point student deficit", () => { + expect(eloExpected(1100, 1500)).toBeCloseTo(0.0909, 3); + }); + + it("is symmetric: eloExpected(a,b) + eloExpected(b,a) === 1", () => { + const a = 1620; + const b = 1430; + expect(eloExpected(a, b) + eloExpected(b, a)).toBeCloseTo(1, 10); + }); + + it("is monotonic: a higher student rating strictly increases the result", () => { + const item = 1500; + const lower = eloExpected(1400, item); + const higher = eloExpected(1600, item); + expect(higher).toBeGreaterThan(lower); + }); +}); + +describe("eloJitter", () => { + it("is deterministic: the same id always returns the same value", () => { + expect(eloJitter(42)).toBe(eloJitter(42)); + expect(eloJitter(123456)).toBe(eloJitter(123456)); + }); + + it("is bounded within [-0.03, 0.03] across hundreds of ids", () => { + for (let id = 0; id < 500; id++) { + const j = eloJitter(id); + expect(j).toBeGreaterThanOrEqual(-0.03); + expect(j).toBeLessThan(0.03); + } + }); + + it("produces different values for different ids (not a constant)", () => { + const values = [1, 2, 3, 4, 5, 100, 999, 123456].map(eloJitter); + const distinct = new Set(values); + expect(distinct.size).toBeGreaterThan(1); + }); +}); + +/** + * The shared mastery-gate decision (issue #549 §5). Both lms_practice_quiz and + * lms_record_practice_attempt route through this, so the rendered session and + * the stored session can never disagree about what is allowed — before #549 + * only the quiz tool gated at all, and the record tool (which its own + * description tells the model to call directly for free-text and chat/voice + * drills) wrote the very rows the gate later reads back to decide readiness. + */ +describe("blockedInterleavingTopics", () => { + const pool = computeInterleavingPool([ + // "loops" clears the gate: 2 attempts >= 70 inside the 90-day window. + { topic: "loops", score: 90, created_at: daysAgo(5) }, + { topic: "loops", score: 75, created_at: daysAgo(3) }, + // "recursion" has only one passing attempt. + { topic: "recursion", score: 95, created_at: daysAgo(4) }, + // "pointers" has two attempts, both failing. + { topic: "pointers", score: 40, created_at: daysAgo(6) }, + { topic: "pointers", score: 55, created_at: daysAgo(2) }, + ]); + + it("blocks nothing when every topic is interleaving-ready", () => { + expect(blockedInterleavingTopics(["loops"], pool)).toEqual([]); + }); + + it("blocks a topic with only one passing attempt", () => { + expect(blockedInterleavingTopics(["recursion"], pool)).toEqual(["recursion"]); + }); + + it("blocks a topic whose attempts all scored below the gate", () => { + expect(blockedInterleavingTopics(["pointers"], pool)).toEqual(["pointers"]); + }); + + it("blocks a topic with no history at all — an unknown topic is never ready", () => { + expect(blockedInterleavingTopics(["never-practised"], pool)).toEqual([ + "never-practised", + ]); + }); + + it("returns every blocked topic in a mixed set, and only those", () => { + expect( + blockedInterleavingTopics(["loops", "recursion", "pointers"], pool) + ).toEqual(["recursion", "pointers"]); + }); + + it("accepts a Set, which is what the tools actually pass", () => { + const topics = new Set(["loops", "never-practised"]); + expect(blockedInterleavingTopics(topics, pool)).toEqual(["never-practised"]); + }); + + it("blocks everything against an empty pool — a novice cannot bootstrap a mixed session", () => { + const empty = computeInterleavingPool([]); + expect(blockedInterleavingTopics(["a", "b"], empty)).toEqual(["a", "b"]); + }); +}); diff --git a/mcp-server/src/tools/practice.ts b/mcp-server/src/tools/practice.ts index 4830544a..173aacba 100644 --- a/mcp-server/src/tools/practice.ts +++ b/mcp-server/src/tools/practice.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "node:crypto"; import { z } from "zod"; import type { MCPServer } from "mcp-use/server"; import { widget, text } from "mcp-use/server"; @@ -242,7 +243,7 @@ type InterleaveTopicStat = { }; /** Pure pool computation over practice_attempts rows (newest-first or any order). */ -function computeInterleavingPool( +export function computeInterleavingPool( rows: Array<{ topic: string; score: number; created_at: string }> ): Map { const cutoff = new Date( @@ -271,6 +272,19 @@ function computeInterleavingPool( return pool; } +/** + * Topics that have NOT cleared the mastery gate. Shared by lms_practice_quiz + * (which gates the rendered session) and lms_record_practice_attempt (which + * gates the stored one) so the two can never drift apart — before #549 only the + * first had a gate at all, and the second was the documented path. + */ +export function blockedInterleavingTopics( + topics: Iterable, + pool: Map +): string[] { + return [...topics].filter((t) => !(pool.get(t)?.ready ?? false)); +} + /** Fetch the caller's interleaving pool, optionally scoped to a course. */ async function fetchInterleavingPool( session: LmsSession, @@ -756,9 +770,7 @@ export function registerPracticeTools(server: MCPServer) { "mode='mixed' needs questions tagged with ≥2 distinct topics (set each question's 'topic'). For one topic, run a focused quiz instead." ); const pool = await fetchInterleavingPool(session, courseId); - const blocked = [...questionTopics].filter( - (t) => !(pool.get(t)?.ready ?? false) - ); + const blocked = blockedInterleavingTopics(questionTopics, pool); if (blocked.length > 0) return errorResult( `Mixed session rejected — these topics haven't passed the interleaving mastery gate yet: ${blocked @@ -797,7 +809,7 @@ export function registerPracticeTools(server: MCPServer) { { name: "lms_record_practice_attempt", description: - "Persist a finished practice-quiz attempt for the caller (practice storage only — never real grades). The practice-player widget calls this automatically for fully closed quizzes; call it yourself after grading free_text answers or a chat/voice drill round. Mixed (interleaved) attempts are split into one stored row per topic — pass mode='mixed' and make sure each question object carries its 'topic' so attribution per topic survives.", + "Persist a finished practice-quiz attempt for the caller (practice storage only — never real grades). The practice-player widget calls this automatically for fully closed quizzes; call it yourself after grading free_text answers or a chat/voice drill round. Mixed (interleaved) attempts are split into one stored row per topic — pass mode='mixed' and make sure each question object carries its 'topic' so attribution per topic survives. mode='mixed' is subject to the same interleaving mastery gate as lms_practice_quiz: it is rejected if any topic is not yet interleaving-ready or the teacher disabled interleaving for the course. Recording a mixed session as focused to dodge the gate is not acceptable — run focused practice on the blocked topics instead.", schema: z.object({ topic: z.string().min(1).describe("What was practiced (session label for mixed sessions)"), mode: z @@ -873,6 +885,37 @@ export function registerPracticeTools(server: MCPServer) { const distinctTopics = new Set(questionTopic.values()); const isMixed = input.mode === "mixed" && distinctTopics.size > 1; + // Mastery gate (#393) — enforced HERE as well as in lms_practice_quiz + // (#549 §5). This tool used to accept mode='mixed' with arbitrary + // per-question topics and no gate and no kill-switch check, and its own + // description tells the model to call it directly for free-text and + // chat/voice drills, so the bypass WAS the documented path. The rows it + // writes are the same rows fetchInterleavingPool() reads back to decide + // readiness, so an ungated session certified its own topics. Both checks + // run before the insert, so a session can never self-certify. + if (isMixed) { + if (!(await isInterleavingEnabled(session, courseId))) + return errorResult( + "Interleaved practice is disabled for this course by the teacher. Record this round as focused single-topic practice instead (omit 'mode')." + ); + const pool = await fetchInterleavingPool(session, courseId); + const blocked = blockedInterleavingTopics(distinctTopics, pool); + if (blocked.length > 0) + return errorResult( + `Mixed attempt rejected — these topics haven't passed the interleaving mastery gate yet: ${blocked + .map((t) => `"${t}"`) + .join( + ", " + )}. A topic becomes interleaving-ready after ${INTERLEAVE_GATE_MIN_ATTEMPTS} practice attempts scoring ≥${INTERLEAVE_GATE_SCORE} in the last ${INTERLEAVE_GATE_WINDOW_DAYS} days. Record focused single-topic practice on the blocked topic(s) first (always allowed), then mix. Do NOT relabel questions to dodge the gate.` + ); + } + + // One session id shared by every per-topic slice, so the XP trigger can + // award once per session instead of once per stored row — a 3-topic + // mixed session used to pay 45 XP and burn 3 of the 10 daily slots + // against 15 XP for the same work done focused (#549 §5). + const sessionId = randomUUID(); + const baseRow = { user_id: session.getUserId(), tenant_id: session.getTenantId(), @@ -880,6 +923,7 @@ export function registerPracticeTools(server: MCPServer) { lesson_id: input.lesson_id ?? null, source_exercise_id: input.source_exercise_id ?? null, source: "mcp-tutor", + session_id: sessionId, }; type TopicSlice = { @@ -946,6 +990,7 @@ export function registerPracticeTools(server: MCPServer) { return ok( { attempt_ids: (data ?? []).map((row) => row.id), + session_id: sessionId, mode: isMixed ? "mixed" : "focused", per_topic: (data ?? []).map((row) => ({ topic: row.topic, @@ -955,7 +1000,7 @@ export function registerPracticeTools(server: MCPServer) { correct_count: input.correct_count, total_questions: input.total_questions, }, - `Practice attempt recorded (${input.correct_count}/${input.total_questions}, score ${input.score})${isMixed ? ` — mixed session split into ${slices.length} per-topic rows` : ""}. XP is awarded automatically.${isMixed ? " Mixed-session accuracy runs lower by design; do NOT lower difficulty because of it — only sustained mastery signals may." : ""}${input.correct_count < input.total_questions ? " Before explaining the missed items, ask the student ONE short question about their reasoning on a miss and tailor the explanation to their answer (one nudge max, skippable)." : ""}` + `Practice attempt recorded (${input.correct_count}/${input.total_questions}, score ${input.score})${isMixed ? ` — mixed session split into ${slices.length} per-topic rows` : ""}. XP is awarded automatically, once per session (a mixed session earns the same as the equivalent focused one — splitting by topic does not multiply it).${isMixed ? " Mixed-session accuracy runs lower by design; do NOT lower difficulty because of it — only sustained mastery signals may." : ""}${input.correct_count < input.total_questions ? " Before explaining the missed items, ask the student ONE short question about their reasoning on a miss and tailor the explanation to their answer (one nudge max, skippable)." : ""}` ); } catch (err) { return errorResult(err instanceof Error ? err.message : String(err)); diff --git a/mcp-server/vitest.config.ts b/mcp-server/vitest.config.ts new file mode 100644 index 00000000..30f19ba0 --- /dev/null +++ b/mcp-server/vitest.config.ts @@ -0,0 +1,18 @@ +import { defineConfig } from 'vitest/config' + +/** + * Test runner for the MCP server package (issue #549 §0). + * + * `mcp-server` is not an npm workspace of the root package and has no vitest of + * its own; npm puts every ancestor `node_modules/.bin` on PATH, so `npm test` + * here resolves the root Vitest binary without adding an install step. Vite + * resolves the package's `.js` specifiers (`../session.js`) back to their `.ts` + * sources, so tests import the tool modules exactly as the server does. + */ +export default defineConfig({ + test: { + environment: 'node', + include: ['src/**/*.test.ts', 'tests/**/*.test.ts'], + globals: false, + }, +}) diff --git a/supabase/functions/spend-points/index.ts b/supabase/functions/spend-points/index.ts index 303f5a10..7eaff223 100644 --- a/supabase/functions/spend-points/index.ts +++ b/supabase/functions/spend-points/index.ts @@ -1,28 +1,37 @@ import { createClient } from "https://esm.sh/@supabase/supabase-js@2"; import { corsHeaders } from "../_shared/cors.ts"; -function getTenantIdFromJwt(authHeader: string): string | null { - try { - const token = authHeader.replace("Bearer ", ""); - const payload = JSON.parse(atob(token.split(".")[1])); - return payload.tenant_id || payload.app_metadata?.tenant_id || null; - } catch { - return null; - } -} - +/** + * Store redemption. + * + * Auth and the plan feature gate happen here; the mutation itself is one call + * to the redeem_store_item() RPC, which runs under a SELECT ... FOR UPDATE on + * the caller's gamification_profiles row (migration 20260726140400, issue #549 + * §6). + * + * This function previously did the work inline as three separate + * read-modify-write cycles — total_coins_spent, streak_freezes_available, and a + * count-then-insert for the max_per_user cap — with no transaction. Because the + * coin balance is derived (floor(total_xp / 10) - total_coins_spent), a lost + * update meant the student kept the item AND the coins, and two parallel + * purchases cost one item's price. Do not reintroduce mutations here: the RPC + * is the only writer, so the lock actually covers everything it needs to. + */ Deno.serve(async (req) => { if (req.method === "OPTIONS") { return new Response("ok", { headers: corsHeaders }); } + const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, + headers: { ...corsHeaders, "Content-Type": "application/json" }, + }); + try { const authHeader = req.headers.get("Authorization"); if (!authHeader) { - return new Response(JSON.stringify({ error: "Missing authorization" }), { - status: 401, - headers: { ...corsHeaders, "Content-Type": "application/json" }, - }); + return json({ error: "Missing authorization" }, 401); } const supabaseUrl = Deno.env.get("SUPABASE_URL")!; @@ -34,10 +43,7 @@ Deno.serve(async (req) => { }); const { data: { user }, error: userError } = await userClient.auth.getUser(); if (userError || !user) { - return new Response(JSON.stringify({ error: "Unauthorized" }), { - status: 401, - headers: { ...corsHeaders, "Content-Type": "application/json" }, - }); + return json({ error: "Unauthorized" }, 401); } const tenantId = getTenantIdFromJwt(authHeader) || "00000000-0000-0000-0000-000000000001"; @@ -46,169 +52,48 @@ Deno.serve(async (req) => { // Check if store feature is enabled for this tenant's plan const { data: features } = await admin.rpc("get_gamification_features", { _tenant_id: tenantId }); if (!features?.store) { - return new Response(JSON.stringify({ error: "Store is not available on your plan", feature_locked: true }), { - status: 403, - headers: { ...corsHeaders, "Content-Type": "application/json" }, - }); + return json({ error: "Store is not available on your plan", feature_locked: true }, 403); } const { item_id } = await req.json(); if (!item_id) { - return new Response(JSON.stringify({ error: "item_id is required" }), { - status: 400, - headers: { ...corsHeaders, "Content-Type": "application/json" }, - }); - } - - // Get store item (global or tenant-specific) - const { data: item, error: itemError } = await admin - .from("gamification_store_items") - .select("*") - .eq("id", item_id) - .eq("is_available", true) - .or(`tenant_id.is.null,tenant_id.eq.${tenantId}`) - .single(); - - if (itemError || !item) { - return new Response(JSON.stringify({ error: "Item not found or inactive" }), { - status: 404, - headers: { ...corsHeaders, "Content-Type": "application/json" }, - }); + return json({ error: "item_id is required" }, 400); } - // Get user profile (tenant-scoped) - const { data: profile, error: profileError } = await admin - .from("gamification_profiles") - .select("total_xp, total_coins_spent") - .eq("user_id", user.id) - .eq("tenant_id", tenantId) - .single(); + // The whole redemption, atomically. `user.id` comes from the verified JWT + // above — the RPC is service-role only and trusts its caller for that. + const { data: result, error: rpcError } = await admin.rpc("redeem_store_item", { + _user_id: user.id, + _tenant_id: tenantId, + _item_id: item_id, + }); - if (profileError || !profile) { - return new Response(JSON.stringify({ error: "Gamification profile not found" }), { - status: 404, - headers: { ...corsHeaders, "Content-Type": "application/json" }, - }); + if (rpcError) { + return json({ error: "Failed to process redemption", detail: rpcError.message }, 500); } - // Calculate available coins - const availableCoins = Math.floor(profile.total_xp / 10) - profile.total_coins_spent; - - if (availableCoins < item.price_coins) { - return new Response( - JSON.stringify({ - error: "Insufficient coins", - available: availableCoins, - required: item.price_coins, - }), - { status: 400, headers: { ...corsHeaders, "Content-Type": "application/json" } } + if (!result?.ok) { + const notFound = result?.code === "item_not_found" || result?.code === "no_profile"; + return json( + { + error: result?.error ?? "Redemption failed", + code: result?.code ?? "unknown", + // Preserved from the previous shape so existing clients keep working. + ...(result?.code === "insufficient_coins" + ? { available: result.available, required: result.required } + : {}), + }, + notFound ? 404 : 400 ); } - // Check max_per_user limit (tenant-scoped). - // For streak_freeze the cap is PER CALENDAR MONTH (issue #394: streak - // saver is capped per month); all other items keep a lifetime cap. - if (item.max_per_user !== null) { - const isMonthlyCap = item.slug === "streak_freeze"; - - let query = admin - .from("gamification_redemptions") - .select("id", { count: "exact", head: true }) - .eq("user_id", user.id) - .eq("tenant_id", tenantId) - .eq("item_id", item_id); - - if (isMonthlyCap) { - const now = new Date(); - const monthStart = new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), 1)).toISOString(); - query = query.gte("redeemed_at", monthStart); - } - - const { count } = await query; - - if ((count || 0) >= item.max_per_user) { - return new Response( - JSON.stringify({ - error: isMonthlyCap - ? "Monthly redemption limit reached for this item" - : "Maximum redemptions reached for this item", - }), - { status: 400, headers: { ...corsHeaders, "Content-Type": "application/json" } } - ); - } - } - - // Create redemption record (tenant-scoped) - const { error: redemptionError } = await admin - .from("gamification_redemptions") - .insert({ - user_id: user.id, - item_id: item_id, - coins_spent: item.price_coins, - tenant_id: tenantId, - }); - - if (redemptionError) { - return new Response(JSON.stringify({ error: "Failed to process redemption" }), { - status: 500, - headers: { ...corsHeaders, "Content-Type": "application/json" }, - }); - } - - // Update total_coins_spent (tenant-scoped) - await admin - .from("gamification_profiles") - .update({ total_coins_spent: profile.total_coins_spent + item.price_coins, updated_at: new Date().toISOString() }) - .eq("user_id", user.id) - .eq("tenant_id", tenantId); - - // Special handling for streak_freeze - if (item.slug === "streak_freeze") { - const { data: currentProfile } = await admin - .from("gamification_profiles") - .select("streak_freezes_available") - .eq("user_id", user.id) - .eq("tenant_id", tenantId) - .single(); - - await admin - .from("gamification_profiles") - .update({ - streak_freezes_available: Math.min((currentProfile?.streak_freezes_available || 0) + 1, 5), - }) - .eq("user_id", user.id) - .eq("tenant_id", tenantId); - } - - // Create user reward record for applicable items (tenant-scoped) - if (["double_xp_1h", "hint_token", "early_access"].includes(item.slug)) { - const expiresAt = item.slug === "double_xp_1h" - ? new Date(Date.now() + 60 * 60 * 1000).toISOString() - : item.slug === "early_access" - ? new Date(Date.now() + 7 * 24 * 60 * 60 * 1000).toISOString() - : null; - - await admin.from("gamification_user_rewards").insert({ - user_id: user.id, - reward_type: item.slug, - reward_data: { item_id: item.id, item_name: item.name }, - expires_at: expiresAt, - is_active: true, - tenant_id: tenantId, - }); - } - - const coinsRemaining = availableCoins - item.price_coins; - - return new Response( - JSON.stringify({ - success: true, - item: { id: item.id, name: item.name, slug: item.slug }, - coins_spent: item.price_coins, - coins_remaining: coinsRemaining, - }), - { headers: { ...corsHeaders, "Content-Type": "application/json" } } - ); + return json({ + success: true, + item: result.item, + coins_spent: result.coins_spent, + coins_remaining: result.coins_remaining, + streak_freezes_available: result.streak_freezes_available, + }); } catch (error) { return new Response(JSON.stringify({ error: (error as Error).message }), { status: 500, @@ -216,3 +101,13 @@ Deno.serve(async (req) => { }); } }); + +function getTenantIdFromJwt(authHeader: string): string | null { + try { + const token = authHeader.replace("Bearer ", ""); + const payload = JSON.parse(atob(token.split(".")[1])); + return payload.tenant_id || payload.app_metadata?.tenant_id || null; + } catch { + return null; + } +} diff --git a/supabase/migrations/20260726140000_league_rollover_catchup_and_bands.sql b/supabase/migrations/20260726140000_league_rollover_catchup_and_bands.sql new file mode 100644 index 00000000..c2d61465 --- /dev/null +++ b/supabase/migrations/20260726140000_league_rollover_catchup_and_bands.sql @@ -0,0 +1,248 @@ +-- ============================================================================= +-- League rollover: missed-tick catch-up + cohort-scaled promotion bands +-- Issue #549 §1 and §2 (epic #540). Fixes 20260717120000_create_weekly_leagues. +-- +-- §1 — One skipped rollover wiped every student's tier. +-- The original resolved the previous week as `v_week - 7` and finalized +-- exactly that one week. Miss a single Monday 00:05 tick (deploy window, or +-- pg_cron absent and nothing external calling /api/cron/league-rollover) and +-- at the next run `v_week - 7` has zero memberships: every student's `pm` +-- join goes NULL, `COALESCE(..., 1)` resets the whole tenant to Bronze, and +-- the genuinely-previous week keeps final_rank/movement NULL forever. +-- +-- Now: the previous week is resolved from the data (the greatest week_start +-- below the target that actually has rows for this tenant), and EVERY +-- unfinalized older week is finalized, oldest first. Critically, each week is +-- ranked over its OWN [w, w+7) XP window — carrying the original's single +-- hardcoded window across a multi-week gap would rank a stale cohort over +-- weeks of unrelated activity and produce confidently wrong ranks. +-- +-- This makes the ordinary weekly run its own catch-up path; no operator +-- needs to backfill a _week_start by hand. +-- +-- §2 — Promotion/relegation carried no information at the minimum cohort size. +-- All five tiers seed promote_count = demote_count = 5 against a cold-start +-- floor of 10 eligible students, so at exactly 10 the promoted and demoted +-- bands tile the cohort and 'stayed' is unreachable — the tier a student +-- holds then says nothing. Bands now scale with cohort size: +-- +-- LEAST(tier's count, GREATEST(1, active_size / 5), (active_size - 1) / 2) +-- +-- The first term keeps the seeded ceiling, the second targets ~20%, and the +-- third guarantees at least one 'stayed' for any cohort of 3 or more. +-- At active_size = 10 that is 2 promoted / 2 demoted / 6 stayed. +-- +-- Zero-XP members were also ranked alongside active ones and tiebroken by +-- membership UUID, so an inactive student could promote over an active one. +-- They are now excluded from the size the bands are computed against and +-- parked as 'stayed' — they still get a final_rank (leaving it NULL would +-- make them re-finalize forever, since final_rank IS NULL is the filter). +-- ============================================================================= + +CREATE OR REPLACE FUNCTION public.rollover_leagues(_tenant_id UUID, _week_start DATE DEFAULT NULL) +RETURNS INTEGER +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path = public +AS $$ +DECLARE + v_week DATE := COALESCE(_week_start, date_trunc('week', now())::date); -- Monday + v_prev_week DATE; + v_pending DATE; + v_settings JSONB; + v_enabled BOOLEAN; + v_min INT; + v_cohort_max INT; + v_max_tier INT; + v_count INT; + v_inserted INT := 0; +BEGIN + -- Idempotency: this week already assigned for this tenant + IF EXISTS ( + SELECT 1 FROM league_memberships + WHERE tenant_id = _tenant_id AND week_start = v_week + ) THEN + RETURN 0; + END IF; + + -- Plan gate: leagues require the leaderboard feature + IF NOT COALESCE((get_gamification_features(_tenant_id) ->> 'leagues')::boolean, false) THEN + RETURN 0; + END IF; + + -- Tenant-level settings (admin disable + cold-start threshold) + SELECT setting_value INTO v_settings + FROM tenant_settings + WHERE tenant_id = _tenant_id AND setting_key = 'leagues'; + + v_enabled := COALESCE((v_settings ->> 'enabled')::boolean, true); + v_min := COALESCE((v_settings ->> 'min_students')::int, 10); + v_cohort_max := COALESCE((v_settings ->> 'cohort_size')::int, 30); + + IF NOT v_enabled THEN + RETURN 0; + END IF; + + SELECT MAX(tier) INTO v_max_tier FROM league_tiers; + + -- --------------------------------------------------------------------------- + -- Finalize EVERY unfinalized past week, oldest first (§1). Each week is + -- ranked over its own [w, w + 7) XP window. + -- --------------------------------------------------------------------------- + FOR v_pending IN + SELECT DISTINCT week_start + FROM league_memberships + WHERE tenant_id = _tenant_id + AND week_start < v_week + AND final_rank IS NULL + ORDER BY week_start + LOOP + WITH xp AS ( + SELECT m.id, m.cohort_id, m.tier, + COALESCE(SUM(t.xp_amount), 0) AS wxp + FROM league_memberships m + LEFT JOIN gamification_xp_transactions t + ON t.user_id = m.user_id + AND t.tenant_id = m.tenant_id + AND t.created_at >= v_pending + AND t.created_at < v_pending + 7 + WHERE m.tenant_id = _tenant_id + AND m.week_start = v_pending + AND m.final_rank IS NULL + GROUP BY m.id, m.cohort_id, m.tier + ), + ranked AS ( + SELECT id, tier, wxp, + ROW_NUMBER() OVER (PARTITION BY cohort_id ORDER BY wxp DESC, id) AS rnk, + -- Bands are sized against ACTIVE members only (§2); zero-XP rows + -- sort last under wxp DESC and are parked below. + COUNT(*) FILTER (WHERE wxp > 0) OVER (PARTITION BY cohort_id) AS active_size + FROM xp + ), + banded AS ( + SELECT r.id, r.tier, r.wxp, r.rnk, r.active_size, + LEAST(lt.promote_count, GREATEST(1, r.active_size / 5), (r.active_size - 1) / 2) AS p_cnt, + LEAST(lt.demote_count, GREATEST(1, r.active_size / 5), (r.active_size - 1) / 2) AS d_cnt + FROM ranked r + JOIN league_tiers lt ON lt.tier = r.tier + ) + UPDATE league_memberships m + SET final_rank = b.rnk, + movement = CASE + -- Inactive members never move on someone else's activity. + WHEN b.wxp <= 0 THEN 'stayed' + WHEN b.rnk <= b.p_cnt AND m.tier < v_max_tier THEN 'promoted' + WHEN b.rnk > GREATEST(b.active_size - b.d_cnt, b.p_cnt) AND m.tier > 1 THEN 'demoted' + ELSE 'stayed' + END + FROM banded b + WHERE m.id = b.id; + END LOOP; + + -- --------------------------------------------------------------------------- + -- Tier continuity follows the last week that ACTUALLY ran, not v_week - 7. + -- NULL (no prior week at all) leaves the LEFT JOIN below unmatched, so a + -- genuine first run still starts everyone at Bronze. + -- --------------------------------------------------------------------------- + SELECT MAX(week_start) INTO v_prev_week + FROM league_memberships + WHERE tenant_id = _tenant_id + AND week_start < v_week; + + -- --------------------------------------------------------------------------- + -- Eligibility: opted-in students with XP activity in the last 14 days + -- --------------------------------------------------------------------------- + DROP TABLE IF EXISTS tmp_league_eligible; + CREATE TEMP TABLE tmp_league_eligible ON COMMIT DROP AS + SELECT gp.user_id, + COALESCE(( + SELECT SUM(t.xp_amount) + FROM gamification_xp_transactions t + WHERE t.user_id = gp.user_id + AND t.tenant_id = _tenant_id + AND t.created_at >= v_week - 14 + ), 0) AS recent_xp + FROM gamification_profiles gp + WHERE gp.tenant_id = _tenant_id + AND gp.leagues_opt_out = false + AND EXISTS ( + SELECT 1 FROM gamification_xp_transactions t2 + WHERE t2.user_id = gp.user_id + AND t2.tenant_id = _tenant_id + AND t2.created_at >= v_week - 14 + ); + + SELECT COUNT(*) INTO v_count FROM tmp_league_eligible; + + -- Cold-start guard: no leagues for tiny tenants + IF v_count < v_min THEN + RETURN 0; + END IF; + + -- --------------------------------------------------------------------------- + -- New week's tier per student: previous tier +/- movement; new joiners at 1 + -- --------------------------------------------------------------------------- + DROP TABLE IF EXISTS tmp_league_assign; + CREATE TEMP TABLE tmp_league_assign ON COMMIT DROP AS + SELECT e.user_id, e.recent_xp, + COALESCE( + CASE pm.movement + WHEN 'promoted' THEN LEAST(pm.tier + 1, v_max_tier) + WHEN 'demoted' THEN GREATEST(pm.tier - 1, 1) + ELSE pm.tier + END, + 1 + ) AS tier + FROM tmp_league_eligible e + LEFT JOIN league_memberships pm + ON pm.tenant_id = _tenant_id + AND pm.user_id = e.user_id + AND pm.week_start = v_prev_week; + + -- --------------------------------------------------------------------------- + -- Cohorts: within each tier, order by recent XP (similar engagement together) + -- and split into balanced groups of <= v_cohort_max + -- --------------------------------------------------------------------------- + WITH counts AS ( + SELECT tier, COUNT(*) AS cnt, + CEIL(COUNT(*)::numeric / v_cohort_max)::int AS n_cohorts + FROM tmp_league_assign + GROUP BY tier + ), + ranked AS ( + SELECT a.user_id, a.tier, + ROW_NUMBER() OVER (PARTITION BY a.tier ORDER BY a.recent_xp DESC, a.user_id) AS rn + FROM tmp_league_assign a + ), + bucketed AS ( + SELECT r.user_id, r.tier, + (((r.rn - 1) * c.n_cohorts) / c.cnt) + 1 AS bucket + FROM ranked r + JOIN counts c USING (tier) + ), + cohort_ids AS ( + SELECT tier, bucket, gen_random_uuid() AS cohort_id + FROM (SELECT DISTINCT tier, bucket FROM bucketed) db + ) + INSERT INTO league_memberships (tenant_id, user_id, week_start, tier, cohort_id) + SELECT _tenant_id, b.user_id, v_week, b.tier, ci.cohort_id + FROM bucketed b + JOIN cohort_ids ci USING (tier, bucket) + ON CONFLICT (tenant_id, user_id, week_start) DO NOTHING; + + GET DIAGNOSTICS v_inserted = ROW_COUNT; + RETURN v_inserted; +END; +$$; + +-- Grants are unchanged from 20260717120000, restated because CREATE OR REPLACE +-- keeps them but an explicit statement documents the service/cron-only contract. +REVOKE ALL ON FUNCTION public.rollover_leagues(UUID, DATE) FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.rollover_leagues(UUID, DATE) TO service_role; + +COMMENT ON FUNCTION public.rollover_leagues(UUID, DATE) IS + 'Weekly league rollover for one tenant. Finalizes every unfinalized prior week ' + '(each over its own [w, w+7) XP window) and carries tiers from the last week ' + 'that actually ran, so a missed cron tick no longer resets the tenant to Bronze. ' + 'Promotion/demotion bands scale with active cohort size and always leave at ' + 'least one "stayed"; zero-XP members are parked, never promoted (issue #549).'; diff --git a/supabase/migrations/20260726140100_digest_candidates_local_day.sql b/supabase/migrations/20260726140100_digest_candidates_local_day.sql new file mode 100644 index 00000000..0f623f33 --- /dev/null +++ b/supabase/migrations/20260726140100_digest_candidates_local_day.sql @@ -0,0 +1,89 @@ +-- ============================================================================= +-- Daily digest candidates: stop pre-filtering streak risk on the UTC day +-- Issue #549 §3 (epic #540). Fixes 20260714150000_daily_digest. +-- +-- The streak-at-risk branch tested `gp.last_activity_date = CURRENT_DATE - 1`, +-- i.e. UTC yesterday, while the send decision and the idempotency key in +-- lib/notifications/daily-digest.ts are both TENANT-LOCAL. Whenever +-- nudge_hour + |utc_offset| >= 24 the two disagree by a day — which is every +-- evening send in the UTC-5 markets this product targets (Bogota, Lima): the +-- 20:00 local nudge runs at 01:00 UTC the next UTC day. +-- +-- This function cannot make the decision correctly, because it does not know +-- the tenant's timezone (that lives in tenant_settings.daily_digest and is +-- resolved per-tenant by the cron). Its job is to be a cheap superset. An +-- equality on the UTC day is not a superset — it excludes exactly the students +-- the local-day comparison would include. +-- +-- Widened to `>= CURRENT_DATE - 2`, which covers local-yesterday for every +-- offset in the UTC-12..UTC+14 range, and the precise call is left to +-- isStreakAtRisk(), which does know the timezone. Rows that turn out not to be +-- at risk are dropped there before anything is sent. +-- +-- award_xp()'s own day boundary is deliberately NOT changed here: it would +-- shift every existing streak. See the note on isStreakAtRisk(). +-- ============================================================================= + +CREATE OR REPLACE FUNCTION public.get_daily_digest_candidates() +RETURNS TABLE ( + tenant_id uuid, + user_id uuid, + email text, + full_name text, + due_cards bigint, + goals_pending bigint, + current_streak integer, + last_activity_date date +) +LANGUAGE sql +STABLE +SECURITY DEFINER +SET search_path = public +AS $$ + SELECT + tu.tenant_id, + tu.user_id, + au.email::text, + p.full_name, + COALESCE(rc.due_count, 0) AS due_cards, + COALESCE(sg.pending_count, 0) AS goals_pending, + COALESCE(gp.current_streak, 0) AS current_streak, + gp.last_activity_date + FROM tenant_users tu + JOIN auth.users au ON au.id = tu.user_id + LEFT JOIN profiles p ON p.id = tu.user_id + LEFT JOIN gamification_profiles gp + ON gp.user_id = tu.user_id AND gp.tenant_id = tu.tenant_id + LEFT JOIN LATERAL ( + SELECT count(*) AS due_count + FROM review_cards rc + WHERE rc.user_id = tu.user_id + AND rc.tenant_id = tu.tenant_id + AND rc.suspended = false + AND rc.due_at <= now() + ) rc ON true + LEFT JOIN LATERAL ( + SELECT count(*) AS pending_count + FROM study_goals sg + WHERE sg.user_id = tu.user_id + AND sg.tenant_id = tu.tenant_id + AND sg.week_start = (date_trunc('week', (now() AT TIME ZONE 'utc')))::date + AND sg.done = false + ) sg ON true + WHERE tu.role = 'student' + AND tu.status = 'active' + AND ( + COALESCE(rc.due_count, 0) > 0 + OR COALESCE(sg.pending_count, 0) > 0 + -- Superset of "streak at risk" across all tenant timezones; the exact + -- local-day comparison happens in isStreakAtRisk(). + OR (COALESCE(gp.current_streak, 0) >= 3 + AND gp.last_activity_date >= CURRENT_DATE - 2) + ); +$$; + +-- Service-role only: the function reads auth.users and crosses tenants. +REVOKE ALL ON FUNCTION public.get_daily_digest_candidates() FROM PUBLIC; +REVOKE ALL ON FUNCTION public.get_daily_digest_candidates() FROM anon; +REVOKE ALL ON FUNCTION public.get_daily_digest_candidates() FROM authenticated; +GRANT EXECUTE ON FUNCTION public.get_daily_digest_candidates() TO service_role; diff --git a/supabase/migrations/20260726140200_fsrs_backfill_lapsed_cards.sql b/supabase/migrations/20260726140200_fsrs_backfill_lapsed_cards.sql new file mode 100644 index 00000000..c6a01d87 --- /dev/null +++ b/supabase/migrations/20260726140200_fsrs_backfill_lapsed_cards.sql @@ -0,0 +1,44 @@ +-- ============================================================================= +-- FSRS backfill: seed the cards the first pass skipped +-- Issue #549 §4 (epic #540). Completes 20260713120000_add_fsrs_to_review_cards. +-- +-- The original seed ran `where repetitions > 0 and stability is null`. But the +-- SM-2 implementation it replaced reset repetitions to 0 on a lapse: +-- +-- case "again": { interval_days: 0, repetitions: 0, ease: ease - 0.2 } +-- +-- so `repetitions > 0` selects exactly the cards that were NOT struggling and +-- skips every card whose most recent rating was "Again" — the ones carrying the +-- lowest ease and the highest FSRS difficulty. cardFromRow() then returns +-- createEmptyCard() for each of them: lapses reset to 0, difficulty reset to the +-- default prior, review history discarded. The original comment claimed +-- "Never-reviewed cards stay New"; lapsed cards had silently joined them. +-- +-- Widened to `repetitions > 0 OR last_reviewed_at IS NOT NULL`. The +-- `stability is null` guard is what keeps this re-runnable and is why rows the +-- first pass already seeded are left completely alone. +-- +-- Two refinements beyond a straight predicate widening, both documented +-- approximations in the same spirit as the original seed (FSRS self-corrects on +-- subsequent reviews): +-- * lapsed cards land in state 3 (Relearning), not 2 (Review) — that is what +-- a card whose last rating was "Again" actually is; +-- * they get lapses = 1, since a reset repetitions count is direct evidence +-- of at least one lapse and 0 would understate difficulty to the scheduler. +-- +-- `ease` is the retained SM-2 column the 20260713120000 comment kept precisely +-- for this; difficulty uses the same linear map (ease 1.3..3.7 -> 10..1). +-- ============================================================================= + +UPDATE public.review_cards +SET fsrs_state = CASE WHEN repetitions > 0 THEN 2 ELSE 3 END, + stability = GREATEST(interval_days, 0.1), + difficulty = LEAST(10, GREATEST(1, 10 - (ease - 1.3) * 3.75)), + lapses = CASE WHEN repetitions > 0 THEN lapses ELSE GREATEST(lapses, 1) END +WHERE (repetitions > 0 OR last_reviewed_at IS NOT NULL) + AND stability IS NULL; + +COMMENT ON COLUMN public.review_cards.stability IS + 'FSRS stability. Seeded from SM-2 interval_days for pre-migration cards, ' + 'including lapsed ones (repetitions = 0 with a last_reviewed_at) — see ' + 'issue #549 for why the first backfill missed those.'; diff --git a/supabase/migrations/20260726140300_practice_attempt_session_xp.sql b/supabase/migrations/20260726140300_practice_attempt_session_xp.sql new file mode 100644 index 00000000..af5ab123 --- /dev/null +++ b/supabase/migrations/20260726140300_practice_attempt_session_xp.sql @@ -0,0 +1,85 @@ +-- ============================================================================= +-- Practice XP: award once per session, not once per stored row +-- Issue #549 §5 (epic #540). Fixes 20260710120000_create_practice_attempts. +-- +-- A mixed (interleaved) session is stored as one practice_attempts row PER +-- TOPIC (#393, migration 20260715120000) so the per-topic pipelines keep +-- aggregating unchanged. But trg_practice_attempt_xp is a FOR EACH ROW trigger +-- awarding 15 XP per row, so a 3-topic session paid 45 XP and burned 3 of the +-- 10 daily slots against 15 XP for the equivalent focused session. XP scaled +-- with how many topics the model chose to tag, not with work done. +-- +-- Fix: rows carry a session_id, set once per lms_record_practice_attempt call +-- across every slice. The trigger awards for the first row of a session and is +-- a no-op for its siblings, keyed on the XP transaction it already writes — so +-- it stays correct on retry and needs no extra bookkeeping table. +-- +-- Also closes the check-then-act on the daily cap noted in the same issue: the +-- count and the award are now serialized per (user, tenant) by a transaction +-- advisory lock, so concurrent inserts can no longer both observe 9 awards and +-- both award. The lock is taken before the count, and released at commit. +-- +-- Existing rows each get their own session_id, which preserves the XP they were +-- already granted (they were each awarded individually at the time). +-- ============================================================================= + +ALTER TABLE public.practice_attempts + ADD COLUMN IF NOT EXISTS session_id UUID NOT NULL DEFAULT gen_random_uuid(); + +COMMENT ON COLUMN public.practice_attempts.session_id IS + 'Groups the per-topic rows of one mixed practice session. XP is awarded once ' + 'per session_id, not once per row (issue #549).'; + +CREATE INDEX IF NOT EXISTS practice_attempts_session_idx + ON public.practice_attempts (session_id); + +CREATE OR REPLACE FUNCTION public.handle_practice_attempt_xp() +RETURNS TRIGGER +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path = public +AS $$ +DECLARE + v_awards_today int; +BEGIN + -- Serialize this user's award path so the cap check below is not a + -- check-then-act race between concurrent inserts. + PERFORM pg_advisory_xact_lock( + hashtextextended(NEW.user_id::text || ':' || NEW.tenant_id::text, 0) + ); + + -- One award per SESSION. The sibling rows of a mixed session find the + -- transaction written by the first row and award nothing. + IF EXISTS ( + SELECT 1 + FROM public.gamification_xp_transactions + WHERE user_id = NEW.user_id + AND tenant_id = NEW.tenant_id + AND action_type = 'practice_attempt' + AND reference_type = 'practice_session' + AND reference_id = NEW.session_id::text + ) THEN + RETURN NEW; + END IF; + + SELECT count(*) INTO v_awards_today + FROM public.gamification_xp_transactions + WHERE user_id = NEW.user_id + AND tenant_id = NEW.tenant_id + AND action_type = 'practice_attempt' + AND created_at >= date_trunc('day', now()); + + IF v_awards_today < 10 THEN + PERFORM award_xp( + NEW.user_id, 'practice_attempt', 15, + NEW.session_id::text, 'practice_session', NEW.tenant_id + ); + END IF; + + RETURN NEW; +END; +$$; + +-- Trigger-only function — not meant to be a PostgREST RPC (advisor: +-- anon_security_definer_function_executable; matches fix_advisor_security_findings). +REVOKE EXECUTE ON FUNCTION public.handle_practice_attempt_xp() FROM public, anon, authenticated; diff --git a/supabase/migrations/20260726140400_redeem_store_item_atomic.sql b/supabase/migrations/20260726140400_redeem_store_item_atomic.sql new file mode 100644 index 00000000..1b98aa31 --- /dev/null +++ b/supabase/migrations/20260726140400_redeem_store_item_atomic.sql @@ -0,0 +1,169 @@ +-- ============================================================================= +-- Store redemption: one locking RPC replaces three unsynchronized writes +-- Issue #549 §6 (epic #540). Backs supabase/functions/spend-points. +-- +-- The edge function performed, with no transaction and no atomic increment: +-- * total_coins_spent = + price +-- * a second independent read-modify-write of streak_freezes_available +-- * a count() followed by an insert for the max_per_user cap +-- +-- The coin balance is DERIVED (floor(total_xp / 10) - total_coins_spent), so a +-- lost update means the student keeps the item AND the coins: two parallel +-- purchases cost one item's price. The same race exceeds the 5-freeze ceiling +-- and the monthly cap that #394 introduced as the anti-farm control. In the +-- other direction, buying a freeze while already at 5 spent the coins, granted +-- nothing (Math.min(existing + 1, 5)) and returned success. +-- +-- Everything now happens under one SELECT ... FOR UPDATE on the profile row: +-- balance read, cap count, redemption insert, both counter updates. The lock is +-- per (user, tenant), so it serializes exactly the redemptions that contend and +-- nothing else. +-- +-- NOTE ON THE UNIQUE INDEX the issue proposes for the monthly cap: it does not +-- fit. streak_freeze is seeded with max_per_user = 5, not 1 +-- (20260216000001_seed_gamification_data.sql), 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. +-- +-- Service-role only: the edge function verifies the caller's JWT itself and +-- then calls with the admin client, where auth.uid() is NULL — hence _user_id +-- as a parameter. This must NOT be reachable by authenticated clients, or a +-- student could grant themselves items. +-- ============================================================================= + +CREATE OR REPLACE FUNCTION public.redeem_store_item( + _user_id UUID, + _tenant_id UUID, + _item_id UUID +) +RETURNS JSONB +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path = public +AS $$ +DECLARE + v_item RECORD; + v_total_xp INT; + v_spent INT; + v_freezes INT; + v_available INT; + v_count INT; + v_window_start TIMESTAMPTZ; + v_expires TIMESTAMPTZ; + v_is_freeze BOOLEAN; +BEGIN + SELECT id, slug, name, price_coins, max_per_user + INTO v_item + FROM gamification_store_items + WHERE id = _item_id + AND is_available = true + AND (tenant_id IS NULL OR tenant_id = _tenant_id); + + IF NOT FOUND THEN + RETURN jsonb_build_object('ok', false, 'code', 'item_not_found', + 'error', 'Item not found or inactive'); + END IF; + + v_is_freeze := v_item.slug = 'streak_freeze'; + + -- --------------------------------------------------------------------------- + -- THE lock. Every read and write below runs under it. + -- --------------------------------------------------------------------------- + SELECT total_xp, total_coins_spent, COALESCE(streak_freezes_available, 0) + INTO v_total_xp, v_spent, v_freezes + FROM gamification_profiles + WHERE user_id = _user_id AND tenant_id = _tenant_id + FOR UPDATE; + + IF NOT FOUND THEN + RETURN jsonb_build_object('ok', false, 'code', 'no_profile', + 'error', 'Gamification profile not found'); + END IF; + + v_available := floor(v_total_xp / 10.0)::int - v_spent; + + IF v_available < v_item.price_coins THEN + RETURN jsonb_build_object('ok', false, 'code', 'insufficient_coins', + 'error', 'Insufficient coins', + 'available', v_available, + 'required', v_item.price_coins); + END IF; + + -- Refuse at the ceiling instead of charging for a grant that cannot land. + IF v_is_freeze AND v_freezes >= 5 THEN + RETURN jsonb_build_object('ok', false, 'code', 'freeze_ceiling', + 'error', 'You already hold the maximum of 5 streak freezes'); + END IF; + + IF v_item.max_per_user IS NOT NULL THEN + -- streak_freeze caps PER CALENDAR MONTH (#394); every other item is + -- lifetime-capped. UTC month start matches the edge function's previous + -- Date.UTC(...) boundary, so the cap window does not shift under this change. + v_window_start := CASE + WHEN v_is_freeze THEN date_trunc('month', (now() AT TIME ZONE 'utc')) AT TIME ZONE 'utc' + ELSE '-infinity'::timestamptz + END; + + SELECT count(*) INTO v_count + FROM gamification_redemptions + WHERE user_id = _user_id + AND tenant_id = _tenant_id + AND item_id = _item_id + AND redeemed_at >= v_window_start; + + IF v_count >= v_item.max_per_user THEN + RETURN jsonb_build_object('ok', false, 'code', 'cap_reached', 'error', + CASE WHEN v_is_freeze + THEN 'Monthly redemption limit reached for this item' + ELSE 'Maximum redemptions reached for this item' END); + END IF; + END IF; + + INSERT INTO gamification_redemptions (user_id, item_id, coins_spent, tenant_id) + VALUES (_user_id, _item_id, v_item.price_coins, _tenant_id); + + -- Atomic increment off the stored value, not off the value read at the top of + -- the request. The freeze grant rides the same UPDATE. + UPDATE gamification_profiles + SET total_coins_spent = total_coins_spent + v_item.price_coins, + streak_freezes_available = CASE + WHEN v_is_freeze THEN COALESCE(streak_freezes_available, 0) + 1 + ELSE streak_freezes_available + END, + updated_at = now() + WHERE user_id = _user_id AND tenant_id = _tenant_id; + + IF v_item.slug IN ('double_xp_1h', 'hint_token', 'early_access') THEN + v_expires := CASE v_item.slug + WHEN 'double_xp_1h' THEN now() + interval '1 hour' + WHEN 'early_access' THEN now() + interval '7 days' + ELSE NULL + END; + + INSERT INTO gamification_user_rewards + (user_id, reward_type, reward_data, expires_at, is_active, tenant_id) + VALUES + (_user_id, v_item.slug, + jsonb_build_object('item_id', v_item.id, 'item_name', v_item.name), + v_expires, true, _tenant_id); + END IF; + + RETURN jsonb_build_object( + 'ok', true, + 'item', jsonb_build_object('id', v_item.id, 'name', v_item.name, 'slug', v_item.slug), + 'coins_spent', v_item.price_coins, + 'coins_remaining', v_available - v_item.price_coins, + 'streak_freezes_available', CASE WHEN v_is_freeze THEN v_freezes + 1 ELSE v_freezes END + ); +END; +$$; + +REVOKE ALL ON FUNCTION public.redeem_store_item(UUID, UUID, UUID) FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.redeem_store_item(UUID, UUID, UUID) TO service_role; + +COMMENT ON FUNCTION public.redeem_store_item(UUID, UUID, UUID) IS + 'Atomic store redemption under a profile row lock: balance check, cap check, ' + 'redemption insert, coin debit and reward grant in one unit. service_role ' + 'only — the caller is responsible for authenticating _user_id (issue #549).'; diff --git a/tests/sql/issue-549/README.md b/tests/sql/issue-549/README.md new file mode 100644 index 00000000..66e8a7a1 --- /dev/null +++ b/tests/sql/issue-549/README.md @@ -0,0 +1,73 @@ +# Seeded-DB acceptance tests — issue #549 + +Assertions for the learning-engine correctness fixes (leagues, FSRS backfill, +practice XP, store redemption). These cover the parts of #549 that live in SQL +and therefore cannot be reached by the Vitest suites: + +| Script | Covers | +|---|---| +| `test-leagues-549.sql` | §1 missed-rollover catch-up + per-week XP window · §2 cohort-scaled bands, zero-XP members parked | +| `test-fsrs-549.sql` | §4 backfill reaches lapsed cards, leaves never-reviewed and already-seeded rows alone | +| `test-practice-xp-549.sql` | §5 a 3-topic mixed session earns the same XP as a 1-topic focused one | +| `test-store-race-549.sql` | §6 sequential correctness, the freeze ceiling refusing without charging, insufficient-coins | +| `setup-race-549.sql` + `verify-race-549.sql` + `cleanup-race-549.sql` | §6 the actual concurrency race (see below) | + +Every script except the race trio runs inside `BEGIN; … ROLLBACK;` and leaves the +database exactly as it found it. Each ends in a `DO $$ … $$` block that raises an +exception on failure and a `PASS:` notice on success, so a failure is loud rather +than something you have to eyeball. + +## Running them + +Requires a local stack (`supabase start`) with all migrations applied. + +```bash +docker exec -i supabase_db_lms-front psql -U postgres -d postgres -P pager=off \ + < tests/sql/issue-549/test-leagues-549.sql +``` + +The `-i` is required — without it psql gets no stdin and silently produces +nothing. Note also that psql's `\i` resolves paths *inside* the container, so +redirect from the host as above rather than using `\i`. + +These use tenant `00000000-0000-0000-0000-000000000002` (Code Academy Pro), which +is on the enterprise plan and therefore passes the gamification feature gates. +Inserting into `auth.users` fires `handle_new_user()`, which already creates the +`profiles` row — hence the `ON CONFLICT` guards in the seeds. + +## The concurrency race (§6) + +This one cannot be a single rolled-back transaction: it needs genuinely +concurrent, committed sessions to show that `redeem_store_item()` serializes on +the profile row lock. It writes real rows, so run the cleanup afterwards. + +A shell function, not a variable — `$PSQL` as a bare variable does not word-split +under zsh, which is the default shell on macOS. + +```bash +psqlx() { docker exec -i supabase_db_lms-front psql -U postgres -d postgres -P pager=off "$@"; } +USER_ID=aaaaaaaa-bbbb-cccc-dddd-eeeeeeee0549 +TENANT=00000000-0000-0000-0000-000000000002 + +psqlx < tests/sql/issue-549/setup-race-549.sql + +# 5 concurrent purchases of double_xp_1h (price 1000, no max_per_user). +ITEM=$(psqlx -tAc "select id from gamification_store_items where slug='double_xp_1h'" | tr -d '[:space:]') +for i in 1 2 3 4 5; do + psqlx -tAc "select redeem_store_item('$USER_ID'::uuid, '$TENANT'::uuid, '$ITEM'::uuid)" & +done +wait + +psqlx < tests/sql/issue-549/verify-race-549.sql +psqlx < tests/sql/issue-549/cleanup-race-549.sql +``` + +Expected: all five calls return `ok: true` with *distinct* `coins_remaining` +values (4000 / 3000 / 2000 / 1000 / 0 — proof they serialized rather than raced), +`total_coins_spent` exactly 5000, and exactly 5 redemption rows. Before the fix +this was three unsynchronized read-modify-writes, so a lost update showed up as +fewer coins spent than redemptions granted. + +`cleanup-race-549.sql` removes the synthetic user's redemptions, gamification +profile, profile and `auth.users` row. Run it — the shared local stack is not +yours alone. diff --git a/tests/sql/issue-549/cleanup-race-549.sql b/tests/sql/issue-549/cleanup-race-549.sql new file mode 100644 index 00000000..e63c63c8 --- /dev/null +++ b/tests/sql/issue-549/cleanup-race-549.sql @@ -0,0 +1,17 @@ +-- Cleanup for §6 part (b) synthetic user. Committed data, no BEGIN/ROLLBACK. +\set ON_ERROR_STOP on + +\set user_id '''aaaaaaaa-bbbb-cccc-dddd-eeeeeeee0549''' +\set tenant '''00000000-0000-0000-0000-000000000002''' + +DELETE FROM gamification_redemptions WHERE user_id = :user_id::uuid AND tenant_id = :tenant::uuid; +DELETE FROM gamification_profiles WHERE user_id = :user_id::uuid AND tenant_id = :tenant::uuid; +DELETE FROM profiles WHERE id = :user_id::uuid; +DELETE FROM auth.users WHERE id = :user_id::uuid; + +\echo '=== Post-cleanup verification: all should be 0 ===' +SELECT + (SELECT count(*) FROM gamification_redemptions WHERE user_id = :user_id::uuid) AS redemptions, + (SELECT count(*) FROM gamification_profiles WHERE user_id = :user_id::uuid) AS gam_profiles, + (SELECT count(*) FROM profiles WHERE id = :user_id::uuid) AS profiles, + (SELECT count(*) FROM auth.users WHERE id = :user_id::uuid) AS auth_users; diff --git a/tests/sql/issue-549/setup-race-549.sql b/tests/sql/issue-549/setup-race-549.sql new file mode 100644 index 00000000..017fcc8f --- /dev/null +++ b/tests/sql/issue-549/setup-race-549.sql @@ -0,0 +1,24 @@ +-- Committed setup for the real concurrency race test (§6 part b). No BEGIN/ROLLBACK. +\set ON_ERROR_STOP on + +\set user_id '''aaaaaaaa-bbbb-cccc-dddd-eeeeeeee0549''' +\set tenant '''00000000-0000-0000-0000-000000000002''' + +INSERT INTO auth.users (id, instance_id, aud, role, email, encrypted_password, + created_at, updated_at, raw_app_meta_data, raw_user_meta_data) +VALUES (:user_id::uuid, '00000000-0000-0000-0000-000000000000', 'authenticated', 'authenticated', + 'storerace549b@test.local', '', now(), now(), '{}'::jsonb, '{}'::jsonb) +ON CONFLICT (id) DO NOTHING; + +INSERT INTO profiles (id, full_name, username) +VALUES (:user_id::uuid, 'Store Race Tester B', 'storerace549b_tester') +ON CONFLICT (id) DO UPDATE SET full_name = EXCLUDED.full_name; + +-- Enough XP for 5 purchases of double_xp_1h (price 1000, no max_per_user): 50000 XP -> 5000 coins. +INSERT INTO gamification_profiles (user_id, tenant_id, total_xp, level, total_coins_spent, streak_freezes_available) +VALUES (:user_id::uuid, :tenant::uuid, 50000, 1, 0, 0) +ON CONFLICT (user_id, tenant_id) DO UPDATE + SET total_xp = EXCLUDED.total_xp, total_coins_spent = 0, streak_freezes_available = 0; + +SELECT 'setup complete' AS status, total_xp, total_coins_spent +FROM gamification_profiles WHERE user_id = :user_id::uuid AND tenant_id = :tenant::uuid; diff --git a/tests/sql/issue-549/test-fsrs-549.sql b/tests/sql/issue-549/test-fsrs-549.sql new file mode 100644 index 00000000..771c50d4 --- /dev/null +++ b/tests/sql/issue-549/test-fsrs-549.sql @@ -0,0 +1,118 @@ +-- Acceptance test for issue #549 §4, run inside a rolled-back transaction. +\set ON_ERROR_STOP on +BEGIN; + +\set tenant '''00000000-0000-0000-0000-000000000002''' + +-- Synthetic user. +CREATE TEMP TABLE u AS SELECT gen_random_uuid() AS id; + +INSERT INTO auth.users (id, instance_id, aud, role, email, encrypted_password, + created_at, updated_at, raw_app_meta_data, raw_user_meta_data) +SELECT id, '00000000-0000-0000-0000-000000000000', 'authenticated', 'authenticated', + 'fsrs549@test.local', '', now(), now(), '{}'::jsonb, '{}'::jsonb +FROM u; + +INSERT INTO profiles (id, full_name, username) +SELECT id, 'FSRS Tester', 'fsrs549_tester' FROM u +ON CONFLICT (id) DO UPDATE SET full_name = EXCLUDED.full_name; + +-- Card A: LAPSED card (repetitions reset to 0 by old SM-2 "again" branch). +INSERT INTO review_cards (id, user_id, tenant_id, front, back, ease, interval_days, + repetitions, last_reviewed_at, stability, difficulty, lapses) +SELECT 900001, u.id, :tenant::uuid, 'Card A front', 'Card A back', 1.7, 0, + 0, now() - interval '3 days', NULL, NULL, 0 +FROM u; + +-- Card A2: an untouched duplicate of A's pre-migration state, used only to +-- demonstrate the ORIGINAL predicate matches zero rows. +INSERT INTO review_cards (id, user_id, tenant_id, front, back, ease, interval_days, + repetitions, last_reviewed_at, stability, difficulty, lapses) +SELECT 900002, u.id, :tenant::uuid, 'Card A2 front', 'Card A2 back', 1.7, 0, + 0, now() - interval '3 days', NULL, NULL, 0 +FROM u; + +-- Card B: NEVER-REVIEWED card. +INSERT INTO review_cards (id, user_id, tenant_id, front, back, ease, interval_days, + repetitions, last_reviewed_at, stability, difficulty, lapses) +SELECT 900003, u.id, :tenant::uuid, 'Card B front', 'Card B back', 2.5, 0, + 0, NULL, NULL, NULL, 0 +FROM u; + +-- Card C: ALREADY-SEEDED card (first backfill pass already ran on it). +INSERT INTO review_cards (id, user_id, tenant_id, front, back, ease, interval_days, + repetitions, last_reviewed_at, stability, difficulty, lapses) +SELECT 900004, u.id, :tenant::uuid, 'Card C front', 'Card C back', 2.9, 10, + 3, now() - interval '1 day', 42.0, 5.0, 0 +FROM u; + +\echo '=== Demonstrate the bug: ORIGINAL predicate against a fresh copy of card A ===' +SELECT count(*) AS old_predicate_matches +FROM review_cards +WHERE id = 900002 AND repetitions > 0 AND stability IS NULL; + +-- Re-run the migration's UPDATE statement verbatim, restricted to our synthetic +-- rows via the id range so we don't touch any pre-existing real data. +UPDATE public.review_cards +SET fsrs_state = CASE WHEN repetitions > 0 THEN 2 ELSE 3 END, + stability = GREATEST(interval_days, 0.1), + difficulty = LEAST(10, GREATEST(1, 10 - (ease - 1.3) * 3.75)), + lapses = CASE WHEN repetitions > 0 THEN lapses ELSE GREATEST(lapses, 1) END +WHERE (repetitions > 0 OR last_reviewed_at IS NOT NULL) + AND stability IS NULL + AND id IN (900001, 900002, 900003, 900004); + +\echo '=== Post-migration state of synthetic cards ===' +SELECT id, repetitions, fsrs_state, stability, difficulty, lapses, ease +FROM review_cards WHERE id IN (900001, 900002, 900003, 900004) ORDER BY id; + +\echo '=== ASSERTIONS ===' +DO $$ +DECLARE + v_old_pred_count INT; + v_a RECORD; + v_b RECORD; + v_c RECORD; + v_expected_difficulty NUMERIC; +BEGIN + SELECT count(*) INTO v_old_pred_count + FROM review_cards WHERE id = 900002 AND repetitions > 0 AND stability IS NULL; + IF v_old_pred_count <> 0 THEN + RAISE EXCEPTION 'FAIL: original predicate matched % rows on a fresh lapsed card, expected 0', v_old_pred_count; + END IF; + + SELECT * INTO v_a FROM review_cards WHERE id = 900001; + IF v_a.stability IS NULL OR v_a.difficulty IS NULL THEN + RAISE EXCEPTION 'FAIL: card A (lapsed) still has NULL stability/difficulty after backfill'; + END IF; + IF v_a.fsrs_state <> 3 THEN + RAISE EXCEPTION 'FAIL: card A fsrs_state = %, expected 3 (Relearning)', v_a.fsrs_state; + END IF; + IF v_a.lapses < 1 THEN + RAISE EXCEPTION 'FAIL: card A lapses = %, expected >= 1', v_a.lapses; + END IF; + + v_expected_difficulty := LEAST(10, GREATEST(1, 10 - (v_a.ease - 1.3) * 3.75)); + IF abs(v_a.difficulty - v_expected_difficulty) > 0.0001 THEN + RAISE EXCEPTION 'FAIL: card A difficulty = % but formula from ease=% gives %', + v_a.difficulty, v_a.ease, v_expected_difficulty; + END IF; + + SELECT * INTO v_b FROM review_cards WHERE id = 900003; + IF v_b.stability IS NOT NULL THEN + RAISE EXCEPTION 'FAIL: card B (never-reviewed) got a non-NULL stability = %', v_b.stability; + END IF; + IF v_b.fsrs_state <> 0 THEN + RAISE EXCEPTION 'FAIL: card B fsrs_state = %, expected 0 (New, untouched)', v_b.fsrs_state; + END IF; + + SELECT * INTO v_c FROM review_cards WHERE id = 900004; + IF v_c.stability IS DISTINCT FROM 42.0 THEN + RAISE EXCEPTION 'FAIL: card C (already-seeded) stability changed to %, expected unchanged 42.0', v_c.stability; + END IF; + + RAISE NOTICE 'PASS: cardA difficulty=% (from ease=%) fsrs_state=% lapses=% | cardB stability=NULL fsrs_state=0 | cardC stability unchanged=42.0 | old predicate matched 0 rows', + v_a.difficulty, v_a.ease, v_a.fsrs_state, v_a.lapses; +END $$; + +ROLLBACK; diff --git a/tests/sql/issue-549/test-leagues-549.sql b/tests/sql/issue-549/test-leagues-549.sql new file mode 100644 index 00000000..2e850042 --- /dev/null +++ b/tests/sql/issue-549/test-leagues-549.sql @@ -0,0 +1,142 @@ +-- Acceptance test for issue #549 §1 + §2, run inside a rolled-back transaction. +\set ON_ERROR_STOP on +BEGIN; + +-- Code Academy Pro is on the enterprise plan, so the leagues feature gate passes. +\set tenant '''00000000-0000-0000-0000-000000000002''' + +CREATE TEMP TABLE t AS +SELECT date_trunc('week', '2026-06-01'::date)::date AS w; + +-- 12 synthetic students. +CREATE TEMP TABLE u AS +SELECT gs AS n, gen_random_uuid() AS id FROM generate_series(1, 12) gs; + +INSERT INTO auth.users (id, instance_id, aud, role, email, encrypted_password, + created_at, updated_at, raw_app_meta_data, raw_user_meta_data) +SELECT id, '00000000-0000-0000-0000-000000000000', 'authenticated', 'authenticated', + 'l549-' || n || '@test.local', '', now(), now(), '{}'::jsonb, '{}'::jsonb +FROM u; + +-- handle_new_user() already inserted profiles for these auth.users. +INSERT INTO profiles (id, full_name, username) +SELECT id, 'League Tester ' || n, 'l549_' || n FROM u +ON CONFLICT (id) DO UPDATE SET full_name = EXCLUDED.full_name; + +INSERT INTO gamification_profiles (user_id, tenant_id, total_xp, level, leagues_opt_out) +SELECT id, :tenant::uuid, 0, 1, false FROM u +ON CONFLICT (user_id, tenant_id) DO UPDATE SET leagues_opt_out = false; + +-- Week W: everyone in one Silver (tier 2) cohort. +INSERT INTO league_memberships (tenant_id, user_id, week_start, tier, cohort_id) +SELECT :tenant::uuid, u.id, t.w, 2, '11111111-1111-1111-1111-111111111111'::uuid +FROM u, t; + +-- XP inside week W's own window: students 1..10 descending, 11 and 12 get none. +-- Student 10 is therefore the lowest-ranked ACTIVE member. +INSERT INTO gamification_xp_transactions (user_id, tenant_id, action_type, xp_amount, created_at) +SELECT u.id, :tenant::uuid, 'test_week_w', (11 - u.n) * 100, t.w + 1 +FROM u, t WHERE u.n <= 10; + +-- Discriminating check on the XP WINDOW fix: a huge award for student 10, but +-- in week W+1, OUTSIDE [W, W+7). If the window still spanned everything up to +-- the target week, student 10 would rank FIRST and be promoted. With the +-- per-week window they must still rank last and be demoted. +INSERT INTO gamification_xp_transactions (user_id, tenant_id, action_type, xp_amount, created_at) +SELECT u.id, :tenant::uuid, 'test_week_w1_noise', 999999, t.w + 8 +FROM u, t WHERE u.n = 10; + +-- Recent activity so all 12 clear the 14-day eligibility window at W+14. +INSERT INTO gamification_xp_transactions (user_id, tenant_id, action_type, xp_amount, created_at) +SELECT u.id, :tenant::uuid, 'test_recent', 5, t.w + 11 +FROM u, t; + +-- Week W+7 is SKIPPED entirely — this is the missed cron tick. +-- Run the rollover for W+14. +SELECT rollover_leagues(:tenant::uuid, (SELECT w + 14 FROM t)) AS inserted; + +\echo '=== §1: week W finalized (was left final_rank NULL forever) ===' +SELECT u.n AS student, m.final_rank, m.movement +FROM league_memberships m JOIN u ON u.id = m.user_id, t +WHERE m.tenant_id = :tenant::uuid AND m.week_start = t.w +ORDER BY m.final_rank; + +\echo '=== §1: tiers at W+14 — must NOT all be Bronze ===' +SELECT m.tier, count(*) AS students +FROM league_memberships m JOIN u ON u.id = m.user_id, t +WHERE m.tenant_id = :tenant::uuid AND m.week_start = t.w + 14 +GROUP BY m.tier ORDER BY m.tier; + +\echo '=== ASSERTIONS ===' +DO $$ +DECLARE + v_w DATE := (SELECT w FROM t); + v_tenant UUID := '00000000-0000-0000-0000-000000000002'; + v_unranked INT; + v_stayed INT; + v_promoted INT; + v_demoted INT; + v_zero_promoted INT; + v_bronze INT; + v_carried INT; + v_student10 TEXT; +BEGIN + -- AC: the skipped week is finalized with correct ranks. + SELECT count(*) INTO v_unranked FROM league_memberships + WHERE tenant_id = v_tenant AND week_start = v_w AND final_rank IS NULL; + IF v_unranked <> 0 THEN + RAISE EXCEPTION 'FAIL §1: % memberships in week W still unfinalized', v_unranked; + END IF; + + SELECT count(*) FILTER (WHERE movement = 'stayed'), + count(*) FILTER (WHERE movement = 'promoted'), + count(*) FILTER (WHERE movement = 'demoted') + INTO v_stayed, v_promoted, v_demoted + FROM league_memberships WHERE tenant_id = v_tenant AND week_start = v_w; + + -- AC: a 10-member (active) cohort produces at least one 'stayed'. + IF v_stayed < 1 THEN + RAISE EXCEPTION 'FAIL §2: no student stayed — bands still tile the cohort'; + END IF; + IF v_promoted <> 2 OR v_demoted <> 2 THEN + RAISE EXCEPTION 'FAIL §2: expected 2 promoted / 2 demoted for active_size=10, got % / %', + v_promoted, v_demoted; + END IF; + + -- AC: zero-XP members are not promoted. + SELECT count(*) INTO v_zero_promoted + FROM league_memberships m + WHERE m.tenant_id = v_tenant AND m.week_start = v_w + AND m.movement = 'promoted' + AND NOT EXISTS ( + SELECT 1 FROM gamification_xp_transactions x + WHERE x.user_id = m.user_id AND x.tenant_id = v_tenant + AND x.created_at >= v_w AND x.created_at < v_w + 7); + IF v_zero_promoted <> 0 THEN + RAISE EXCEPTION 'FAIL §2: % zero-XP member(s) promoted', v_zero_promoted; + END IF; + + -- The window check: student 10's out-of-week 999999 XP must not have moved them. + SELECT m.movement INTO v_student10 + FROM league_memberships m + WHERE m.tenant_id = v_tenant AND m.week_start = v_w AND m.final_rank = 10; + IF v_student10 <> 'demoted' THEN + RAISE EXCEPTION 'FAIL §1: XP window leaked across weeks — rank-10 student is %', v_student10; + END IF; + + -- AC: tiers are preserved across the missed tick, not reset to Bronze. + SELECT count(*) FILTER (WHERE tier = 1), count(*) FILTER (WHERE tier <> 1) + INTO v_bronze, v_carried + FROM league_memberships WHERE tenant_id = v_tenant AND week_start = v_w + 14; + IF v_bronze <> 2 THEN + RAISE EXCEPTION 'FAIL §1: expected exactly 2 Bronze (the demoted pair), got % — tier wipe', v_bronze; + END IF; + IF v_carried <> 10 THEN + RAISE EXCEPTION 'FAIL §1: expected 10 students to carry a non-Bronze tier, got %', v_carried; + END IF; + + RAISE NOTICE 'PASS: stayed=% promoted=% demoted=% bronze_at_W+14=% carried=%', + v_stayed, v_promoted, v_demoted, v_bronze, v_carried; +END $$; + +ROLLBACK; diff --git a/tests/sql/issue-549/test-practice-xp-549.sql b/tests/sql/issue-549/test-practice-xp-549.sql new file mode 100644 index 00000000..f3a8443a --- /dev/null +++ b/tests/sql/issue-549/test-practice-xp-549.sql @@ -0,0 +1,104 @@ +-- Acceptance test for issue #549 §5, run inside a rolled-back transaction. +\set ON_ERROR_STOP on +BEGIN; + +\set tenant '''00000000-0000-0000-0000-000000000002''' + +CREATE TEMP TABLE u AS SELECT gen_random_uuid() AS id; + +INSERT INTO auth.users (id, instance_id, aud, role, email, encrypted_password, + created_at, updated_at, raw_app_meta_data, raw_user_meta_data) +SELECT id, '00000000-0000-0000-0000-000000000000', 'authenticated', 'authenticated', + 'practicexp549@test.local', '', now(), now(), '{}'::jsonb, '{}'::jsonb +FROM u; + +INSERT INTO profiles (id, full_name, username) +SELECT id, 'Practice XP Tester', 'practicexp549_tester' FROM u +ON CONFLICT (id) DO UPDATE SET full_name = EXCLUDED.full_name; + +INSERT INTO gamification_profiles (user_id, tenant_id, total_xp, level) +SELECT id, :tenant::uuid, 0, 1 FROM u +ON CONFLICT (user_id, tenant_id) DO UPDATE SET total_xp = EXCLUDED.total_xp; + +-- Mixed session: 3 topics, ONE shared session_id. +CREATE TEMP TABLE s AS SELECT gen_random_uuid() AS session_id; + +INSERT INTO practice_attempts (user_id, tenant_id, topic, questions, answers, score, + total_questions, correct_count, source, mode, session_id) +SELECT u.id, :tenant::uuid, topic, '[]'::jsonb, '[]'::jsonb, 80.0, 5, 4, 'mcp-tutor', 'mixed', s.session_id +FROM u, s, (VALUES ('algebra'), ('geometry'), ('trigonometry')) AS topics(topic); + +\echo '=== After 3-row mixed session (one session_id) ===' +SELECT action_type, xp_amount, reference_id, reference_type +FROM gamification_xp_transactions +WHERE user_id = (SELECT id FROM u) AND tenant_id = :tenant::uuid AND action_type = 'practice_attempt'; + +\echo '=== ASSERTIONS: after mixed session ===' +DO $$ +DECLARE + v_user UUID := (SELECT id FROM u); + v_tenant UUID := '00000000-0000-0000-0000-000000000002'; + v_count_after_mixed INT; + v_amount INT; + v_attempt_rows INT; +BEGIN + SELECT count(*) INTO v_attempt_rows FROM practice_attempts WHERE user_id = v_user AND tenant_id = v_tenant; + IF v_attempt_rows <> 3 THEN + RAISE EXCEPTION 'setup error: expected 3 practice_attempts rows after mixed-session insert, got %', v_attempt_rows; + END IF; + + SELECT count(*), max(xp_amount) INTO v_count_after_mixed, v_amount + FROM gamification_xp_transactions + WHERE user_id = v_user AND tenant_id = v_tenant AND action_type = 'practice_attempt'; + + IF v_count_after_mixed <> 1 THEN + RAISE EXCEPTION 'FAIL: expected exactly 1 XP transaction after a 3-topic mixed session, got %', v_count_after_mixed; + END IF; + IF v_amount <> 15 THEN + RAISE EXCEPTION 'FAIL: expected 15 XP for the mixed session, got %', v_amount; + END IF; + + RAISE NOTICE 'PASS (mixed): % XP transaction(s) totaling % XP for a 3-topic mixed session', v_count_after_mixed, v_amount; +END $$; + +-- Focused session: 1 more row, DIFFERENT session_id. +CREATE TEMP TABLE s2 AS SELECT gen_random_uuid() AS session_id; + +INSERT INTO practice_attempts (user_id, tenant_id, topic, questions, answers, score, + total_questions, correct_count, source, mode, session_id) +SELECT u.id, :tenant::uuid, 'algebra', '[]'::jsonb, '[]'::jsonb, 90.0, 5, 4, 'mcp-tutor', 'focused', s2.session_id +FROM u, s2; + +\echo '=== After the additional focused session (second session_id) ===' +SELECT action_type, xp_amount, reference_id, reference_type +FROM gamification_xp_transactions +WHERE user_id = (SELECT id FROM u) AND tenant_id = :tenant::uuid AND action_type = 'practice_attempt'; + +\echo '=== ASSERTIONS: after focused session ===' +DO $$ +DECLARE + v_user UUID := (SELECT id FROM u); + v_tenant UUID := '00000000-0000-0000-0000-000000000002'; + v_count_after_focused INT; + v_total_xp INT; +BEGIN + SELECT count(*) INTO v_count_after_focused + FROM gamification_xp_transactions + WHERE user_id = v_user AND tenant_id = v_tenant AND action_type = 'practice_attempt'; + + IF v_count_after_focused <> 2 THEN + RAISE EXCEPTION 'FAIL: expected exactly 2 XP transactions after adding a focused session, got %', v_count_after_focused; + END IF; + + SELECT sum(xp_amount) INTO v_total_xp + FROM gamification_xp_transactions + WHERE user_id = v_user AND tenant_id = v_tenant AND action_type = 'practice_attempt'; + + IF v_total_xp <> 30 THEN + RAISE EXCEPTION 'FAIL: expected 30 total XP (15 per session x 2 sessions), got %', v_total_xp; + END IF; + + RAISE NOTICE 'PASS (focused added): % transactions / % XP total (a 3-topic mixed session cost the same as a 1-topic focused session)', v_count_after_focused, v_total_xp; +END $$; + +ROLLBACK; diff --git a/tests/sql/issue-549/test-store-race-549.sql b/tests/sql/issue-549/test-store-race-549.sql new file mode 100644 index 00000000..3c1c17b0 --- /dev/null +++ b/tests/sql/issue-549/test-store-race-549.sql @@ -0,0 +1,137 @@ +-- Acceptance test for issue #549 §6 part (a): sequential correctness + ceiling. +-- Rolled back — does not touch persistent data. +\set ON_ERROR_STOP on +BEGIN; + +\set tenant '''00000000-0000-0000-0000-000000000002''' + +CREATE TEMP TABLE u AS SELECT gen_random_uuid() AS id; + +INSERT INTO auth.users (id, instance_id, aud, role, email, encrypted_password, + created_at, updated_at, raw_app_meta_data, raw_user_meta_data) +SELECT id, '00000000-0000-0000-0000-000000000000', 'authenticated', 'authenticated', + 'storerace549a@test.local', '', now(), now(), '{}'::jsonb, '{}'::jsonb +FROM u; + +INSERT INTO profiles (id, full_name, username) +SELECT id, 'Store Race Tester A', 'storerace549a_tester' FROM u +ON CONFLICT (id) DO UPDATE SET full_name = EXCLUDED.full_name; + +-- Enough XP for many purchases: 100000 XP -> 10000 coins (needs 2500 for 5 freezes). +INSERT INTO gamification_profiles (user_id, tenant_id, total_xp, level, total_coins_spent, streak_freezes_available) +SELECT id, :tenant::uuid, 100000, 1, 0, 0 FROM u +ON CONFLICT (user_id, tenant_id) DO UPDATE + SET total_xp = EXCLUDED.total_xp, total_coins_spent = 0, streak_freezes_available = 0; + +\set item_freeze '''f7cd45e7-ff89-4122-a45e-d3252950dd75''' + +\echo '=== Purchase 1: buy streak_freeze (price 500) ===' +SELECT redeem_store_item((SELECT id FROM u), :tenant::uuid, :item_freeze::uuid) AS result \gset r1_ +\echo :r1_result + +DO $$ +DECLARE + v_user UUID := (SELECT id FROM u); + v_tenant UUID := '00000000-0000-0000-0000-000000000002'; + v_result JSONB; + v_spent_before INT; + v_spent_after INT; + v_freezes_before INT; + v_freezes_after INT; +BEGIN + SELECT total_coins_spent, streak_freezes_available INTO v_spent_before, v_freezes_before + FROM gamification_profiles WHERE user_id = v_user AND tenant_id = v_tenant; + + IF v_spent_before <> 500 OR v_freezes_before <> 1 THEN + RAISE EXCEPTION 'FAIL: after purchase 1 expected spent=500 freezes=1, got spent=% freezes=%', v_spent_before, v_freezes_before; + END IF; + RAISE NOTICE 'PASS: purchase 1 -> total_coins_spent=%, streak_freezes_available=%', v_spent_before, v_freezes_before; +END $$; + +\echo '=== Drive freezes to the ceiling (5), then attempt a 6th ===' +DO $$ +DECLARE + v_user UUID := (SELECT id FROM u); + v_tenant UUID := '00000000-0000-0000-0000-000000000002'; + v_item UUID := 'f7cd45e7-ff89-4122-a45e-d3252950dd75'; + v_result JSONB; + v_spent_before INT; + v_spent_after INT; + v_freezes INT; +BEGIN + -- We're at 1 freeze; buy 4 more to reach 5. + FOR i IN 1..4 LOOP + v_result := redeem_store_item(v_user, v_tenant, v_item); + IF (v_result->>'ok')::boolean IS NOT TRUE THEN + RAISE EXCEPTION 'FAIL: expected purchase % to succeed while below ceiling, got %', i, v_result; + END IF; + END LOOP; + + SELECT total_coins_spent, streak_freezes_available INTO v_spent_before, v_freezes + FROM gamification_profiles WHERE user_id = v_user AND tenant_id = v_tenant; + + IF v_freezes <> 5 THEN + RAISE EXCEPTION 'FAIL: expected streak_freezes_available=5 before ceiling attempt, got %', v_freezes; + END IF; + + -- Attempt the 6th purchase: must be refused, must NOT charge coins. + v_result := redeem_store_item(v_user, v_tenant, v_item); + + IF (v_result->>'ok')::boolean IS NOT FALSE THEN + RAISE EXCEPTION 'FAIL: expected ok=false at the freeze ceiling, got %', v_result; + END IF; + IF (v_result->>'code') <> 'freeze_ceiling' THEN + RAISE EXCEPTION 'FAIL: expected code=freeze_ceiling, got %', v_result->>'code'; + END IF; + + SELECT total_coins_spent INTO v_spent_after + FROM gamification_profiles WHERE user_id = v_user AND tenant_id = v_tenant; + + IF v_spent_after <> v_spent_before THEN + RAISE EXCEPTION 'FAIL: ceiling-refused purchase changed total_coins_spent from % to %', v_spent_before, v_spent_after; + END IF; + + RAISE NOTICE 'PASS: ceiling refusal ok=false code=freeze_ceiling, total_coins_spent unchanged at %', v_spent_after; +END $$; + +\echo '=== Insufficient-coins purchase ===' +DO $$ +DECLARE + v_user UUID := (SELECT id FROM u); + v_tenant UUID := '00000000-0000-0000-0000-000000000002'; + v_item UUID := '38067a72-7dfc-4ba3-875a-0d6b51d7f5d4'; -- double_xp_1h, price 1000 + v_result JSONB; + v_spent_before INT; + v_spent_after INT; + v_redemptions_before INT; + v_redemptions_after INT; +BEGIN + -- Zero out XP so the user can't afford the 1000-coin item. + UPDATE gamification_profiles SET total_xp = 0 WHERE user_id = v_user AND tenant_id = v_tenant; + + SELECT total_coins_spent INTO v_spent_before FROM gamification_profiles WHERE user_id = v_user AND tenant_id = v_tenant; + SELECT count(*) INTO v_redemptions_before FROM gamification_redemptions WHERE user_id = v_user AND tenant_id = v_tenant; + + v_result := redeem_store_item(v_user, v_tenant, v_item); + + IF (v_result->>'ok')::boolean IS NOT FALSE THEN + RAISE EXCEPTION 'FAIL: expected ok=false for insufficient coins, got %', v_result; + END IF; + IF (v_result->>'code') <> 'insufficient_coins' THEN + RAISE EXCEPTION 'FAIL: expected code=insufficient_coins, got %', v_result->>'code'; + END IF; + + SELECT total_coins_spent INTO v_spent_after FROM gamification_profiles WHERE user_id = v_user AND tenant_id = v_tenant; + SELECT count(*) INTO v_redemptions_after FROM gamification_redemptions WHERE user_id = v_user AND tenant_id = v_tenant; + + IF v_spent_after <> v_spent_before THEN + RAISE EXCEPTION 'FAIL: insufficient-coins purchase changed total_coins_spent from % to %', v_spent_before, v_spent_after; + END IF; + IF v_redemptions_after <> v_redemptions_before THEN + RAISE EXCEPTION 'FAIL: insufficient-coins purchase inserted a redemption row (% -> %)', v_redemptions_before, v_redemptions_after; + END IF; + + RAISE NOTICE 'PASS: insufficient_coins refused cleanly, no state change (spent=%, redemptions=%)', v_spent_after, v_redemptions_after; +END $$; + +ROLLBACK; diff --git a/tests/sql/issue-549/verify-race-549.sql b/tests/sql/issue-549/verify-race-549.sql new file mode 100644 index 00000000..4d144bc2 --- /dev/null +++ b/tests/sql/issue-549/verify-race-549.sql @@ -0,0 +1,47 @@ +-- Verification for §6 part (b). Committed data, no BEGIN/ROLLBACK — read-only. +\set ON_ERROR_STOP on + +\set user_id '''aaaaaaaa-bbbb-cccc-dddd-eeeeeeee0549''' +\set tenant '''00000000-0000-0000-0000-000000000002''' + +\echo '=== 5 concurrent redeem_store_item results ===' +-- (results were printed individually by each background psql process) + +\echo '=== Post-race profile state ===' +SELECT total_xp, total_coins_spent, streak_freezes_available +FROM gamification_profiles WHERE user_id = :user_id::uuid AND tenant_id = :tenant::uuid; + +\echo '=== Redemption rows for the double_xp_1h item ===' +SELECT count(*) AS redemption_count, sum(coins_spent) AS sum_coins_spent +FROM gamification_redemptions +WHERE user_id = :user_id::uuid AND tenant_id = :tenant::uuid + AND item_id = '38067a72-7dfc-4ba3-875a-0d6b51d7f5d4'::uuid; + +DO $$ +DECLARE + v_user UUID := 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeee0549'; + v_tenant UUID := '00000000-0000-0000-0000-000000000002'; + v_spent INT; + v_redemption_count INT; + v_expected INT := 5 * 1000; +BEGIN + SELECT total_coins_spent INTO v_spent + FROM gamification_profiles WHERE user_id = v_user AND tenant_id = v_tenant; + + SELECT count(*) INTO v_redemption_count + FROM gamification_redemptions + WHERE user_id = v_user AND tenant_id = v_tenant + AND item_id = '38067a72-7dfc-4ba3-875a-0d6b51d7f5d4'::uuid; + + IF v_redemption_count <> 5 THEN + RAISE EXCEPTION 'FAIL: expected 5 gamification_redemptions rows from 5 concurrent purchases, got %', v_redemption_count; + END IF; + + IF v_spent <> v_expected THEN + RAISE EXCEPTION 'FAIL (LOST UPDATE): total_coins_spent = % but expected % (5 x 1000). redemption_count=%', + v_spent, v_expected, v_redemption_count; + END IF; + + RAISE NOTICE 'PASS: 5 concurrent redemptions -> total_coins_spent=% (expected %), redemption_count=%', + v_spent, v_expected, v_redemption_count; +END $$; diff --git a/tests/unit/daily-digest.test.ts b/tests/unit/daily-digest.test.ts index df080f5d..6feb0282 100644 --- a/tests/unit/daily-digest.test.ts +++ b/tests/unit/daily-digest.test.ts @@ -6,12 +6,12 @@ import { isStreakAtRisk, localDateStr, localHour, + localYesterday, pickTemplate, renderTemplate, resolveChannels, resolveDigestSettings, tenantBaseUrl, - utcYesterday, } from '@/lib/notifications/daily-digest' /** @@ -92,31 +92,89 @@ describe('localDateStr', () => { }) }) -describe('utcYesterday + isStreakAtRisk', () => { +describe('localYesterday + isStreakAtRisk (UTC baseline)', () => { + // These cases predate #549 and were originally written against UTC via the + // removed `utcYesterday`. Passing 'UTC' as the 5th arg to isStreakAtRisk + // preserves their exact original intent. const now = new Date('2026-07-14T12:00:00Z') it('true when last_activity_date is UTC yesterday and streak >= min', () => { - expect(isStreakAtRisk('2026-07-13', 7, now, 7)).toBe(true) + expect(isStreakAtRisk('2026-07-13', 7, now, 7, 'UTC')).toBe(true) }) it('false when last_activity_date is today', () => { - expect(isStreakAtRisk('2026-07-14', 7, now, 7)).toBe(false) + expect(isStreakAtRisk('2026-07-14', 7, now, 7, 'UTC')).toBe(false) }) it('false when last_activity_date is two days ago', () => { - expect(isStreakAtRisk('2026-07-12', 7, now, 7)).toBe(false) + expect(isStreakAtRisk('2026-07-12', 7, now, 7, 'UTC')).toBe(false) }) it('false when streak is below min even if the date matches', () => { - expect(isStreakAtRisk('2026-07-13', 2, now, 7)).toBe(false) + expect(isStreakAtRisk('2026-07-13', 2, now, 7, 'UTC')).toBe(false) }) it('false when last_activity_date is null', () => { - expect(isStreakAtRisk(null, 7, now, 7)).toBe(false) + expect(isStreakAtRisk(null, 7, now, 7, 'UTC')).toBe(false) }) - it('utcYesterday returns YYYY-MM-DD 24h before now', () => { - expect(utcYesterday(now)).toBe('2026-07-13') + it('localYesterday(now, "UTC") returns YYYY-MM-DD 24h before now', () => { + expect(localYesterday(now, 'UTC')).toBe('2026-07-13') + }) +}) + +describe('LATAM regression (issue #549 §3): nudge fires past UTC midnight', () => { + // Bogota is UTC-5 with no DST. A 20:00-local nudge (nudgeHour = 20) fires at + // 01:00 UTC on the *next* UTC calendar day. Before #549, isStreakAtRisk + // compared last_activity_date against UTC-yesterday (via the removed + // utcYesterday), which is off by one day for any tenant west of UTC once + // nudgeHour + |utc_offset| >= 24 — exactly this case (20 + 5 = 25). + // + // now = 2026-03-11T01:00:00Z == 2026-03-10 20:00 in America/Bogota. + const now = new Date('2026-03-11T01:00:00Z') + + it('a student who practised the same Bogota-local day (2026-03-10) is NOT at risk', () => { + // Under the old UTC-yesterday comparison this returned true: UTC-yesterday + // of 2026-03-11T01:00:00Z is 2026-03-10, which incorrectly matched + // last_activity_date and told the student their streak was ending hours + // after they had already secured it. + expect(isStreakAtRisk('2026-03-10', 10, now, 7, 'America/Bogota')).toBe(false) + }) + + it('a student who skipped the Bogota-local day (last active 2026-03-09) IS at risk', () => { + // Under the old UTC-yesterday comparison this returned false: '2026-03-09' + // never matched UTC-yesterday ('2026-03-10'), so the student who actually + // needed the nudge — the one who skipped the local day — was silently + // skipped. + expect(isStreakAtRisk('2026-03-09', 10, now, 7, 'America/Bogota')).toBe(true) + }) + + it('demonstrates the exact off-by-one-day: Bogota-yesterday vs UTC-yesterday diverge at this instant', () => { + expect(localYesterday(now, 'America/Bogota')).toBe('2026-03-09') + expect(localYesterday(now, 'UTC')).toBe('2026-03-10') + }) +}) + +describe('localYesterday DST correctness', () => { + // localYesterday deliberately takes the local calendar date first and steps + // back one calendar day on that date alone, rather than subtracting 24h + // from the instant and re-projecting into the timezone. The latter is wrong + // near a DST transition. + // + // US spring-forward 2026: America/New_York jumps from 2:00 EST to 3:00 EDT + // at 2026-03-08T07:00:00Z (clocks skip 2:00-3:00 local). + // + // now = 2026-03-09T04:30:00Z == 2026-03-09 00:30 EDT (UTC-4, post-transition). + // The correct "yesterday" is the calendar date 2026-03-08. + // + // A naive now-minus-24h approach would compute 2026-03-08T04:30:00Z, which + // is BEFORE that day's 07:00Z transition and therefore still EST (UTC-5): + // 04:30 - 5h = 2026-03-07T23:30 local, i.e. it would wrongly land on + // 2026-03-07 — one calendar day too far back, because the 24h subtraction + // silently ate the extra hour DST added back on March 9. + it('returns the correct previous calendar date across a spring-forward transition', () => { + const now = new Date('2026-03-09T04:30:00Z') + expect(localYesterday(now, 'America/New_York')).toBe('2026-03-08') }) })