Repository navigation
feat: gamification & leaderboard system - #2405
diegosouzapw merged 18 commits into
Conversation
Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
Add migration 060 with 8 tables: leaderboard, user_levels, badge_definitions, user_badges, xp_audit_log, token_ledger, invite_tokens, community_servers. Add gamification.ts domain module with all CRUD functions and re-export from localDb.ts. Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
Wave 4: Add token transfer sharing module, invite/redeem system, and API routes for leaderboard, transfer, and invite operations. - sharing.ts: transfer tokens between API keys with idempotency - invites.ts: create, redeem, list, and revoke invite codes - leaderboard route: GET with scope, limit, and neighbor context - transfer route: GET balance/history, POST transfer - invite route: GET list, POST create, DELETE revoke - invite/redeem route: POST redeem invite code Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
…ages Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive gamification and leaderboard system, including XP/level progression, badge achievements, token sharing via a double-entry ledger, and community server federation. Key features include real-time leaderboard updates via SSE and an anti-cheat validation engine. Feedback highlights several critical issues: a logic error in leaderboard synchronization that causes score inflation, a security vulnerability in the federation API due to missing token validation, and a potential race condition in token transfers. Additionally, the reviewer noted style guide violations regarding error handling in SSE streams and the use of raw SQL outside of domain modules, as well as the need for client-side idempotency keys and improved frontend SSE reconnection logic.
| // Merge into local leaderboard | ||
| const { updateScore } = await import("./leaderboard"); | ||
| for (const entry of data.entries) { | ||
| updateScore(entry.apiKeyId, "global", entry.score); |
There was a problem hiding this comment.
The syncLeaderboard function calls updateScore, which performs an atomic increment (score = score + excluded.score) in the database. This means every time a sync runs, the remote scores are added to the local scores instead of synchronizing them. Repeated syncs will cause scores to inflate incorrectly. You should use a separate function that overwrites the score or takes the maximum of the two.
| const authHeader = request.headers.get("Authorization"); | ||
| if (!authHeader?.startsWith("Bearer ")) { | ||
| return NextResponse.json( | ||
| { error: "Missing authorization" }, | ||
| { status: 401, headers: CORS_HEADERS } | ||
| ); | ||
| } |
There was a problem hiding this comment.
This federation endpoint only checks if the Authorization header starts with Bearer but never validates the token against the stored apiKeyHash of connected community servers. This allows any client to submit arbitrary scores for any apiKeyId. You should extract the token, hash it, and verify it exists in the community_servers table.
| export function transferTokens( | ||
| fromId: string, | ||
| toId: string, | ||
| amount: number, | ||
| reason: string, | ||
| idempotencyKey: string | ||
| ): { success: boolean; error?: string } { | ||
| // Check for duplicate | ||
| const existing = db() | ||
| .prepare(`SELECT id FROM token_ledger WHERE idempotency_key = ?`) | ||
| .get(idempotencyKey) as { id: number } | undefined; | ||
| if (existing) return { success: true }; | ||
|
|
||
| // Balance check | ||
| const balance = getBalance(fromId); | ||
| if (balance < amount) { | ||
| return { success: false, error: "insufficient_balance" }; | ||
| } | ||
|
|
||
| db() | ||
| .prepare( | ||
| `INSERT INTO token_ledger (from_api_key_id, to_api_key_id, amount, reason, idempotency_key) | ||
| VALUES (?, ?, ?, ?, ?)` | ||
| ) | ||
| .run(fromId, toId, amount, reason, idempotencyKey); | ||
|
|
||
| return { success: true }; | ||
| } |
There was a problem hiding this comment.
The transferTokens function performs a balance check followed by an insertion as separate operations. This is prone to race conditions where concurrent transfers could result in a negative balance. These operations must be wrapped in a database transaction to ensure atomicity.
| es.onerror = () => { | ||
| es.close(); | ||
| }; |
There was a problem hiding this comment.
The onerror handler closes the EventSource without any reconnection logic. If the connection is lost due to a transient network issue or server restart, real-time updates will stop permanently until the user manually refreshes the page. Consider implementing an exponential backoff reconnection strategy or using a library that handles this automatically.
| const entries = await getTopN(scope, 50); | ||
| const data = JSON.stringify({ type: "leaderboard", scope, entries }); | ||
| controller.enqueue(encoder.encode(`data: ${data}\n\n`)); | ||
| } catch { |
There was a problem hiding this comment.
Swallowing errors in the SSE stream violates the Repository Style Guide.
| } catch { | |
| } catch (err) { | |
| controller.error(err); | |
| } |
References
- Never silently swallow errors in SSE streams — propagate them or abort the stream cleanly. (link)
| const scoreRow = d | ||
| .prepare("SELECT score FROM leaderboard WHERE api_key_id = ? AND scope = ?") | ||
| .get(apiKeyId, scope) as { score: number } | undefined; | ||
|
|
||
| if (!scoreRow) return { above: [], below: [] }; | ||
|
|
||
| const above = d | ||
| .prepare( | ||
| `SELECT api_key_id, score FROM leaderboard | ||
| WHERE scope = ? AND score > ? | ||
| ORDER BY score ASC LIMIT ?` | ||
| ) | ||
| .all(scope, scoreRow.score, radius) as Array<{ api_key_id: string; score: number }>; | ||
|
|
||
| const below = d | ||
| .prepare( | ||
| `SELECT api_key_id, score FROM leaderboard | ||
| WHERE scope = ? AND score < ? | ||
| ORDER BY score DESC LIMIT ?` | ||
| ) | ||
| .all(scope, scoreRow.score, radius) as Array<{ api_key_id: string; score: number }>; | ||
|
|
There was a problem hiding this comment.
Writing raw SQL in this module violates the Repository Style Guide. All database logic should be encapsulated within src/lib/db/ domain modules.
References
- Never write raw SQL in routes — always go through src/lib/db/ domain modules. (link)
| return { success: false, idempotencyKey: "", error: "Amount must be positive" }; | ||
| } | ||
|
|
||
| const idempotencyKey = crypto.randomUUID(); |
There was a problem hiding this comment.
Generating the idempotencyKey inside the transferTokens function defeats its purpose. If a client retries a request due to a network timeout, a new UUID will be generated for the same logical operation, allowing the transfer to be processed multiple times. The idempotency key should be generated by the client and passed as an argument to this function.
…ites, anti-cheat, events Add 25 tests across 5 new test files covering: - Leaderboard: updateScore, getRank, getTopN, getNeighbors - Sharing: transferTokens validation, getBalance, getHistory - Invites: createInvite, redeemInvite, listInvites, revokeInvite - Anti-Cheat: validateScoreChange, getAnomalies - Events: emitGamificationEvent with valid/invalid/unknown inputs Also add events.ts module for emitting gamification events to xp_audit_log. Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
1. Federation sync: use INSERT ON CONFLICT overwrite instead of additive increment 2. Federation auth: validate Bearer token against community_servers.api_key_hash 3. Token transfer: wrap balance check + insert in database transaction 4. Leaderboard: move raw SQL to DB module (getLeaderboardNeighbors, rotateLeaderboardScope) 5. SSE stream: emit error events instead of swallowing 6. Sharing: accept optional idempotencyKey parameter 7. Leaderboard page: add SSE reconnection on error Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
Review Feedback AddressedCommit
🤖 Generated with OpenClaude |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| : `month_${new Date().toISOString().slice(0, 7)}`; | ||
|
|
||
| const instance = getDbInstance(); | ||
| instance.exec(` |
There was a problem hiding this comment.
CRITICAL: rotateLeaderboardScope can lose data if run twice in the same period. INSERT OR IGNORE silently skips already-archived records, then DELETE removes the original data. Running rotation twice in the same week/month will permanently lose scores that were added after the first rotation but match already-archived api_key_id entries.
-- If run twice in same week, second run:
-- 1. INSERT OR IGNORE: skips entries already archived (same api_key_id + archiveSuffix PK)
-- 2. DELETE: removes ALL entries with scope='weekly' (including new ones added since first rotation)
-- Result: New scores added between rotation #1 and #2 are lost.Consider: (1) checking if rotation already ran for this period before executing, (2) using a separate archive table, or (3) wrapping in a transaction with a rotation timestamp check.
| : `month_${new Date().toISOString().slice(0, 7)}`; | ||
|
|
||
| const instance = getDbInstance(); | ||
| instance.exec(` |
There was a problem hiding this comment.
WARNING: instance.exec() with string interpolation is a SQL injection risk pattern. While archiveSuffix and scope are currently internally generated, using exec() with interpolated values is fragile. If this code is ever modified to accept external input, it will be vulnerable. Use parameterized queries instead.
| status: string; | ||
| error_message: string | null; | ||
| }>; | ||
| return rows.map((r) => ({ |
There was a problem hiding this comment.
WARNING: listServers returns apiKeyHash to the caller. The hashed API key should never be exposed to clients. This field is used for authentication between federated servers and leaking it could allow an attacker to impersonate a connected server.
The CommunityServer interface should exclude apiKeyHash, or listServers should omit it from the returned objects.
| }; | ||
|
|
||
| const XP_PER_LEVEL = 1000; | ||
|
|
There was a problem hiding this comment.
WARNING: Hardcoded XP_PER_LEVEL = 1000 doesn't match the actual XP system. The real system uses a polynomial curve: xpForLevel(n) = floor(100 * n^1.5) from src/lib/gamification/xp.ts. This means the XP progress bar on the profile page will show completely incorrect values.
For example, level 2 requires 252 XP (not 1000), level 10 requires 31,622 cumulative XP. The modulo-based progress calculation on line 121 (totalXp % XP_PER_LEVEL) is meaningless with the actual curve.
Import and use xpForLevel and cumulativeXpForLevel from @/lib/gamification/xp instead.
|
|
||
| const XP_PER_LEVEL = 1000; | ||
|
|
||
| function getTier(level: number): string { |
There was a problem hiding this comment.
WARNING: Duplicated getTier function with different thresholds than src/lib/gamification/xp.ts:getLevelTier(). The profile page uses: level 50=diamond, 30=platinum, 20=gold, 10=silver. The canonical xp.ts uses: level 76=diamond, 51=platinum, 26=gold, 11=silver. Users will see different tier labels on the profile page vs what the backend computes.
Import and use getLevelTier from @/lib/gamification/xp instead of duplicating.
| return "bronze"; | ||
| } | ||
|
|
||
| function getLevelTitle(level: number): string { |
There was a problem hiding this comment.
WARNING: Duplicated getLevelTitle function with different titles and thresholds than src/lib/gamification/xp.ts:getLevelTitle(). The profile page uses: "Diamond Master", "Platinum Architect", "Gold Engineer", "Silver Developer", "Bronze Builder", "Newcomer". The canonical xp.ts uses: "Legend", "Master", "Expert", "Explorer", "Beginner". Users will see inconsistent titles across the app.
Import and use getLevelTitle from @/lib/gamification/xp instead of duplicating.
| // Count total actions of this type | ||
| const row = db | ||
| .prepare( | ||
| "SELECT COALESCE(SUM(xp_earned), 0) AS count FROM xp_audit_log WHERE api_key_id = ? AND action = ?" |
There was a problem hiding this comment.
WARNING: checkActionCountBadges uses SUM(xp_earned) instead of COUNT(*) to count actions. While this currently works because XP per action is 1 for both "request" and "token_share", it's semantically wrong and will break if XP values change. Badge thresholds are meant to count number of actions (e.g., "Made 1,000 API requests"), not total XP earned from those actions.
-- Current (wrong): counts XP earned, not actions
SELECT COALESCE(SUM(xp_earned), 0) AS count ...
-- Should be: counts actual number of action events
SELECT COUNT(*) AS count ...| * Query params: | ||
| * scope — one of: global, weekly, monthly, tokens_shared, contributions (default: global) | ||
| */ | ||
| export async function GET(request: NextRequest) { |
There was a problem hiding this comment.
SUGGESTION: The SSE stream endpoint has no authentication or rate limiting. Any unauthenticated client can open unlimited SSE connections, each triggering a getTopN DB query every 5 seconds. This could be exploited for resource exhaustion (CPU, DB connections, memory from open streams). Consider adding at least basic rate limiting (e.g., max connections per IP) or requiring API key authentication.
| */ | ||
| export async function GET(request: NextRequest) { | ||
| const url = new URL(request.url); | ||
| const scope = (url.searchParams.get("scope") || "global") as any; |
There was a problem hiding this comment.
SUGGESTION: scope is cast with as any which bypasses TypeScript type safety. An attacker could pass arbitrary scope values like "global'; DROP TABLE..." and they would reach the DB query. The getTopN function in src/lib/gamification/leaderboard.ts accepts LeaderboardScope type but receives any here.
Validate the scope against the VALID_SCOPES set (same pattern used in stream/route.ts) before passing to getTopN.
Code Review SummaryStatus: 9 Issues Found (2 CRITICAL, 6 WARNING, 1 SUGGESTION) | Recommendation: Address before merge Overview
Resolved Issues (Fixed in this diff)
NEW Issues (in this diff)
Carried Forward (unchanged files)
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Reviewed by qwen3.6-plus · 362,319 tokens |
Comprehensive SDD for the gamification & leaderboard system covering XP/levels, badges, streaks, leaderboards, token sharing, invite/redeem, community server federation, anti-cheat, API routes, MCP tools, dashboard pages, pipeline integration, security, testing, file structure, and phased migration strategy. Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
| export function recordBadgeUnlock(apiKeyId: string, event: BadgeUnlockEvent): void { | ||
| if (!recentUnlocks.has(apiKeyId)) { | ||
| recentUnlocks.set(apiKeyId, []); | ||
| } |
There was a problem hiding this comment.
WARNING: Memory leak — recentUnlocks Map entries are never removed for stale apiKeyIds
The recentUnlocks Map only trims events within each key's list (via BUFFER_TTL_MS and MAX_BUFFER_SIZE), but the key itself is never deleted once created. In a long-running server with many unique users, this Map will grow indefinitely as orphaned entries accumulate for API keys that no longer have active connections.
Add a periodic cleanup interval that removes keys with empty lists, or use a TTL-based Map implementation.
| const interval = setInterval(() => { | ||
| const events = consumeBadgeUnlocks(apiKeyId); | ||
| for (const event of events) { | ||
| controller.enqueue( |
There was a problem hiding this comment.
WARNING: Unhandled error if controller.enqueue() is called after stream closes
If the client disconnects abruptly (network drop, browser close), the AbortSignal may not fire immediately. The setInterval at line 63 will continue firing and attempt to enqueue() on an already-closed controller, throwing an unhandled error that could crash the process.
Wrap controller.enqueue() in try/catch, or check controller.desiredSize !== null before enqueueing.
| const cleanup = () => { | ||
| clearInterval(interval); | ||
| clearInterval(heartbeat); | ||
| controller.close(); |
There was a problem hiding this comment.
WARNING: controller.close() called after stream is already closed will throw
If signal.addEventListener('abort') fires AND one of the intervals fires before cleanup runs, controller.close() may be called twice. The second call throws TypeError: Controller is already closed.
Guard with a closed flag: track whether cleanup has already run to prevent double-closing.
| const addToast = useCallback((event: BadgeUnlockEvent) => { | ||
| setToasts((prev) => [...prev, event]); | ||
| // Auto-dismiss after 5s | ||
| setTimeout(() => { |
There was a problem hiding this comment.
WARNING: setTimeout is never cleared on unmount — causes state update on unmounted component
If BadgeToast unmounts while toasts are still visible (e.g., user navigates away), the setTimeout callback at line 25 will still fire after 5 seconds and call setToasts() on an unmounted component. This causes a React warning and potential memory leak.
Return a cleanup function from addToast or use useRef to track timeouts and clear them in the useEffect cleanup.
| } | ||
| }); | ||
|
|
||
| es.onerror = () => { |
There was a problem hiding this comment.
WARNING: No reconnect logic for EventSource — permanent disconnect on error
When es.onerror fires (network drop, server restart, proxy timeout), the EventSource is closed permanently with no reconnection attempt. Users would need to manually refresh the page to resume receiving badge notifications.
The leaderboard page (dashboard/leaderboard/page.tsx:70-78) implements reconnect with 3s delay + exponential backoff. This component should follow the same pattern for consistency and reliability.
| const { recordBadgeUnlock } = await import("./notifications"); | ||
| recordBadgeUnlock(apiKeyId, { | ||
| badgeId, | ||
| badgeName: badgeId, |
There was a problem hiding this comment.
WARNING: badgeName set to raw badgeId — users see internal IDs instead of display names
At line 128, badgeName: badgeId passes the raw badge ID (e.g., "first-token", "unstoppable") directly to the notification. Users will see these internal identifiers in toast notifications instead of human-readable names like "First Steps" or "Unstoppable".
Badge definitions with proper display names exist in the constants/badges system. Import the badge definition to get the actual name field before recording the notification.
| /** | ||
| * GET /api/gamification/notifications?apiKeyId=xxx — SSE badge unlock notifications | ||
| */ | ||
| export async function GET(request: NextRequest) { |
There was a problem hiding this comment.
SUGGESTION: No authentication or rate limiting on SSE notification endpoint
This endpoint accepts any apiKeyId query parameter and creates an SSE stream without verifying that the caller owns that API key. An attacker could subscribe to any user's badge notifications, potentially revealing sensitive information about their activity.
At minimum, validate that the authenticated session matches the requested apiKeyId. Consider adding rate limiting on SSE connection creation to prevent resource exhaustion.
- SQL injection: use parameterized INSERT instead of string interpolation - Double-run protection: skip rotation if archive scope already has data - listServers: exclude api_key_hash from SELECT (security leak) - Profile page: import xpForLevel/calculateLevel/getLevelTier/getLevelTitle instead of hardcoded XP_PER_LEVEL and duplicated local functions - checkActionCountBadges: COUNT(*) instead of SUM(xp_earned) - badgeName: look up from badge_definitions instead of using raw badgeId - notifications.ts: cleanup stale Map entries, safe enqueue with try/catch - BadgeToast: track setTimeout IDs and clear on unmount, reconnect with exponential backoff on EventSource error - SSE stream + notifications endpoints: add requireManagementAuth - Federation route: use LeaderboardScope type instead of `as any`
All Review Feedback Addressed (commit f33aa36)Critical
Warnings
Suggestions
All 15 items fixed. 16 tests pass. 🤖 Generated with OpenClaude |
|
|
||
| // Count total actions of this type | ||
| const row = db | ||
| .prepare("COALESCE(COUNT(*), 0) AS count FROM xp_audit_log WHERE api_key_id = ? AND action = ?") |
There was a problem hiding this comment.
CRITICAL: Missing SELECT keyword in SQL statement — will cause runtime error
The SELECT keyword was accidentally removed. The statement "COALESCE(COUNT(*), 0) AS count FROM..." is not valid SQL and better-sqlite3 will throw when preparing it.
| .prepare("COALESCE(COUNT(*), 0) AS count FROM xp_audit_log WHERE api_key_id = ? AND action = ?") | |
| .prepare("SELECT COALESCE(COUNT(*), 0) AS count FROM xp_audit_log WHERE api_key_id = ? AND action = ?") |
|
|
||
| function connect() { | ||
| if (unmounted) return; | ||
| es = new EventSource(`/api/gamification/notifications?apiKeyId=${apiKeyId}`); |
There was a problem hiding this comment.
WARNING: apiKeyId not URL-encoded in EventSource URL
If apiKeyId contains special characters (e.g., +, &, =, spaces), the URL will be malformed and the connection will fail or query the wrong key.
| es = new EventSource(`/api/gamification/notifications?apiKeyId=${apiKeyId}`); | |
| es = new EventSource(`/api/gamification/notifications?apiKeyId=${encodeURIComponent(apiKeyId)}`); |
| */ | ||
| export async function GET(request: NextRequest) { | ||
| const url = new URL(request.url); | ||
| const scope: LeaderboardScope = (url.searchParams.get("scope") || "global") as LeaderboardScope; |
There was a problem hiding this comment.
WARNING: scope cast bypasses runtime validation
The as LeaderboardScope cast only affects compile-time types. Any arbitrary string from the URL (e.g., scope=malicious) will pass through unchecked to getTopN(). This worked before with as any — the type cast changed but the validation gap remains.
Consider validating against known scopes before use, similar to how /stream does it with VALID_SCOPES.
| import { useState, useEffect, useCallback } from "react"; | ||
| import { Card, Badge } from "@/shared/components"; | ||
| import { | ||
| xpForLevel, |
There was a problem hiding this comment.
WARNING: Unused import xpForLevel
xpForLevel is imported but never referenced in this file. Will trigger a lint warning.
| xpForLevel, | |
| cumulativeXpForLevel, |
| import { Card, Badge } from "@/shared/components"; | ||
| import { | ||
| xpForLevel, | ||
| calculateLevel, |
There was a problem hiding this comment.
WARNING: Unused import calculateLevel
calculateLevel is imported but never used in this file. Only cumulativeXpForLevel, getLevelTier, and getLevelTitle are referenced. Will trigger a lint warning.
| calculateLevel, | |
| cumulativeXpForLevel, |
| log.info("events.badge_unlocked", { apiKeyId, badgeId }); | ||
|
|
||
| // Look up badge details from badge_definitions | ||
| const { getDbInstance } = await import("../db/core"); |
There was a problem hiding this comment.
SUGGESTION: Direct SQL in non-db module violates project guidelines
Per AGENTS.md: "No raw SQL in routes — always use domain module functions" and "Direct prepare() statements outside db/ modules — breaks modularity". This queries badge_definitions directly instead of going through the db module's getBadgeDefinitions() function.
Consider adding a getBadgeById(id) function to src/lib/db/gamification.ts and importing it here instead.
81fb3f5
into
cheaper-inference:release/v3.8.0
|
Thank you for the contribution! We have analyzed and merged the gamification and leaderboard system into the release branch. |
Integrated into release/v3.8.0
Integrated into release/v3.8.0
Integrated into release/v3.8.0
Summary
Implements a complete gamification & leaderboard system for OmniRoute (closes #2403).
7 commits, 6 waves:
Features
/dashboard/leaderboard,/dashboard/profile,/dashboard/tokensTechnical
requireManagementAuthon all routesTest plan
npm run typecheck:core— passesnpm run lint— passes🤖 Generated with OpenClaude