Repository navigation
feat(middlewareNode): currency ledger + leaderboard swap (Karthik + Jimmy's Rev. 2 lanes) - #250
Merged
Merged
Conversation
Implements the infra/ledger workstream from the currency rollout plan: - ActionEvent/ActionRule/LedgerEntry/UserBalance schemas, with lifetimeEarned tracked separately from spendable balance so a future spend feature can't make the leaderboard reward not-spending. - A dependency-free MongoDB polling consumer (no Redis) with atomic claim-based processing and crash recovery via stale-claim requeue. - An idempotent ledger writer verified under real concurrent-write conditions, plus cooldown/daily-cap abuse limits checked against the ledger itself (not event status, which only reflects the calling consumer's own lifecycle). - An hourly anomaly-monitoring job (MAD-based, flag-only) that catches a student earning at an outlier rate across many actions combined, which no single action's cooldown/cap can see on its own. - Rate limiting on /gameResults, the current event-triggering endpoint. - A historical backfill script, deliberately scoped to game results only — lessonsCompleted has no per-completion timestamp and timeTrackings' puzzle records measure time spent, not puzzles solved, so backfilling currency from either would mean fabricating history rather than reconstructing it. - Removed PUT /user/updateHighScore: a client-writable score-forging endpoint with zero real callers, found during the readiness audit. 306 tests passing, zero regressions against the existing suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Score Completes the previous commit: server.js now actually starts the currency consumer after connectDB() resolves (skipped under Jest), adds the currencyEventLimiter on /gameResults, and users.js has PUT /updateHighScore removed as described in the prior commit message — these two files' changes were dropped from that commit despite being staged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ev. 2 lane) Implements the producers/leaderboard workstream from the currency rollout plan, built against Karthik's real schemas/consumer (already merged) rather than a temporary stub sink: - services/currencyEvents.js: the single choke point every feature route goes through to record a currency-earning action. Deterministic eventIds (e.g. lesson:<userId>:<piece>:<lessonNum>) so a retried request can never double-emit. - Wired lesson.completed into routes/lessons.js's /updateLessonCompletion, emitting only on a genuine forward-progress write (modifiedCount > 0), never on the existing 304 no-op branch or for unauthenticated guests (no account to credit). - puzzle.solved is deliberately NOT wired: there is no server-side puzzle-completion endpoint anywhere in the codebase to attach to (puzzles.js only lists/serves puzzles). Documented as blocked on a missing feature rather than fabricating an emit point. - routes/leaderboard.js: score now reads UserBalance.lifetimeEarned (batched via a new getLifetimeEarnedMap, one query for the whole candidate page rather than per-student) instead of the old time/ streak/badge/activity weighted formula. A student with no UserBalance document reads as 0, not undefined. Chess record stays a separate, untouched stat. - Rewrote leaderboard.test.js and leaderboard.analytics-consistency.test.js against the new score source — the latter's original premise (leaderboard and analytics should agree via the same formula) no longer holds now that they're deliberately different signals, so it's rewritten to verify the leaderboard genuinely reads real ledger data instead. 326 tests passing, zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
karthikeya1976
changed the title
The Row.score JSDoc still described the old time/streak/activities/ badges weighted formula this session's leaderboard swap replaced. Updated to describe what score actually is now (currency lifetime earned) — no behavior change, the response contract and UI are unaffected since the field name/type/shape didn't change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 of 12 tasks
JimmyWu7
self-requested a review
October 1, 2026 21:45
Addresses review feedback on this PR: POST /puzzles/solved previously awarded currency based solely on an authenticated caller POSTing a puzzleId that existed in the catalog, with no proof the puzzle was actually solved. Any authenticated user could loop over every puzzleId in the catalog and farm currency for puzzles never attempted. - routes/puzzles.js: the route now requires a `moves` array (the UCI move sequence the player submitted) and verifies it positionally against the puzzle's stored `moves` answer key before emitting puzzle.solved. moveMatches() mirrors Puzzles.tsx's own handlePlayerMove leniency exactly (a 4-char move is accepted against a 5-char/promotion solution) so a move the client UI already treated as correct can't be rejected server-side as a mismatch. - Puzzles.tsx: adds playedMovesRef to accumulate the player's verified moves as they're confirmed correct (moveListRef shrinks via .shift() as the puzzle progresses, so it's empty by completion and can't be used for this). Wires a new reportPuzzleSolved() call at the existing "puzzle completed" point, fire-and-forget so a network hiccup doesn't block the success-modal UX. Skipped entirely for guests, matching the backend's own "no account to credit" reasoning. - Tests: added coverage for the move-mismatch rejection (the actual regression case), partial/missing/empty moves, and both directions of the promotion leniency (4-char submission against a 5-char solution accepted; wrong promotion piece at the same length rejected). Noted limitation, not fully closed by this fix: puzzle.moves (the answer key) is already sent to the client to drive the interactive play-through, so a client that reads it directly and submits it verbatim without playing will still pass. This raises the bar from "needs nothing" to "needs the real solution string" — closing the remaining gap would need either not shipping the full solution to the client up front, or a different completion-proof mechanism entirely; flagging as further follow-up rather than silently claiming this is a complete fix. 336/336 backend tests passing (330 baseline + 6 new), TypeScript compiles clean on the frontend change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
feat: emit currency events for lessons and puzzles
…n a duplicate event Resolves the last open item from Karthik's ledger plan's Week 0 gate: the LEDGER_USE_TRANSACTIONS=true branch had code but had never actually been exercised by a test, since every other test in this suite runs against a standalone mongodb-memory-server instance (which doesn't support transactions at all). Added tests/ledgerService.transactions.test.js, using MongoMemoryReplSet (a real, if minimal, replica set) to actually run that branch. Doing so immediately surfaced a genuine bug: the old code caught a duplicate-key error INSIDE session.withTransaction()'s callback and returned normally. The MongoDB driver's withTransaction() retries its callback on a transient error, and apparently cannot distinguish "the callback decided to do nothing" from "the callback needs retrying" when nothing was actually committed -- in an isolated repro this produced hundreds of silent retries of the same duplicate insert before eventually failing a test's timeout, rather than the near-instant no-op the single-document fallback already achieves for the same case. Fix: let the duplicate-key error propagate OUT of the transaction callback entirely. This aborts the (otherwise-empty) transaction immediately -- cheap and correct, since nothing else was written -- and is caught once, after withTransaction() itself settles, mirroring the try/catch shape the non-transactional fallback already uses. This does not answer the actual Week 0 question of whether the real deployment's MongoDB is a replica set -- that still needs someone to run rs.status() against it and report back; I have no access to that database myself (confirmed: the only mongoURI on this machine points at a now-unreachable/deleted Atlas cluster). What it does mean is that flipping LEDGER_USE_TRANSACTIONS=true, whenever that's confirmed safe, is now exercising genuinely tested code instead of an unverified branch that would have hung in production exactly the way it hung in this test before the fix. 343/343 tests passing (336 baseline + 7 new). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fix(ledger): add real transaction-path tests, found and fixed a retry bug
JimmyWu7
reviewed
Oct 2, 2026
JimmyWu7
left a comment
JimmyWu7
left a comment
Collaborator
There was a problem hiding this comment.
Pushed two small fixes after reviewing the CI failure:
- Added
claimedto theActionEventstatus enum to match the consumer's atomic claim/requeue flow. - Updated
currencyEvents.test.jsto wait for theActionEventindexes to initialize before testing duplicate event IDs, which fixes the CI index initialization race.
Local middleware suite passes: 30/30 test suites, 343/343 tests.
JimmyWu7
approved these changes
Oct 2, 2026
ToldYO
self-requested a review
October 7, 2026 14:45
Deepesh-Katudia
added a commit
that referenced
this pull request
Oct 8, 2026
Conflicts: - package.json: keep main's engines.node ">=24 <25" from the Node 24 pin. - StreakModal.tsx: keep the branch's `?react` SVG imports, take main's removal of the calendar placeholder, and import environment from `environments` rather than `environments/environment` (which hardcodes the dev URLs in production). Also fix the same environments/environment import that #250's activitiesApi.ts brought in without a conflict.
8 of 14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the infra/ledger workstream, the producers/leaderboard workstream, and the lesson/puzzle currency-event producers from the currency rollout plan (Rev. 2) — the event-sourced points ledger that replaces the leaderboard's weighted engagement score.
What's in this PR
Karthik's lane — infra & ledger
ActionEvent,ActionRule,LedgerEntry,UserBalance—UserBalancetrackslifetimeEarnedseparately from spendablebalanceso a future spend feature can't make the leaderboard reward not spending./gameResults.users.lessonsCompletedhas no per-completion timestamp andtimeTrackings' puzzle records measure time spent, not puzzles solved.PUT /user/updateHighScore: a client-writable score-forging endpoint with zero real callers.LEDGER_USE_TRANSACTIONS=truehad code but no test coverage. Added real tests against an in-memory replica set and found/fixed a genuine bug in the process — the old code could retry a duplicate-event transaction indefinitely instead of failing fast. See the commit for details.Jimmy's lane — producers & leaderboard
services/currencyEvents.js: the single choke point every feature route goes through to record a currency-earning action. DeterministiceventIds so a retried request can never double-emit.lesson.completedwired into/updateLessonCompletion, emitting only on genuine forward progress (modifiedCount > 0), never on the 304 no-op branch or for guests.puzzle.solved—POST /puzzles/solved, requiring and verifying the player's actual move sequence against the puzzle's stored solution before awarding currency. (An earlier version accepted a barepuzzleIdwith no solve verification — caught in review and fixed before merge; see feat: emit currency events for lessons and puzzles #262.)scorenow readsUserBalance.lifetimeEarned(batched viagetLifetimeEarnedMap) instead of the old weighted formula. Missing balance reads as0, notundefined. Chess record stays untouched.leaderboard.test.jsandleaderboard.analytics-consistency.test.jsagainst the new score source.Testing
mongodb-memory-server, including a real in-memory replica set for the transactional path) covering rules-engine correctness, idempotency under concurrent writes, consumer claim atomicity/crash recovery, backfill idempotency, anomaly detection edge cases, event-producer idempotency, lesson-completion emit guards, and puzzle-completion move verification.npm ci+npm test, not just re-reading prior output.Notes for reviewers
ActionRuleyet (deferred).rs.status()against it yet and reported back — I don't have access to that database myself. The code defaults safely to a non-transactional single-document fallback either way; the transactional branch is now fully tested (see above) so flippingLEDGER_USE_TRANSACTIONS=truelater is a config change, not a leap of faith.puzzle.moves(the solution) is sent to the client to drive the interactive play-through UI, so a client that reads it directly and replays it verbatim without solving would still pass verification. Flagged as further follow-up, not claimed as solved.🤖 Generated with Claude Code