diff --git a/documentation/audits/generating-decks.md b/documentation/audits/generating-decks.md deleted file mode 100644 index 58a2479..0000000 --- a/documentation/audits/generating-decks.md +++ /dev/null @@ -1,371 +0,0 @@ -# Code Review: `build-top-english-nouns-deck` seed script - -Hey, good work getting this to a finished, working state — that's genuinely the hardest part. Below is feedback structured the way a mentor would give it: what the problem is, why it matters in a real codebase, and how to fix it. Work through these one by one when you refactor. - ---- - -## 1. Function names should be imperative, not gerunds - -### What you wrote - -```ts -const readingFromWordlist = async () => { ... } -const checkingSourceWordsAgainstDB = async () => { ... } -``` - -### Why it's a problem - -Functions represent _actions_. In English, imperative verbs describe actions: `read`, `fetch`, `build`. Gerunds (`reading`, `checking`) describe ongoing processes — they read like you're narrating what's happening rather than declaring what a function does. This isn't just style preference: when you're scanning a call stack or reading `main()`, imperative names parse faster because they match the mental model of "I am calling this to do a thing." - -### How to fix it - -```ts -const readWordlist = async () => { ... } -const resolveSourceTerms = async () => { ... } // "checking" undersells what it returns -const writeMissingWords = async () => { ... } -``` - -Note the rename of `checkingSourceWordsAgainstDB` → `resolveSourceTerms`. The original name describes the _mechanism_ (checking against DB). A better name describes the _result_ (resolving words into term IDs). Callers don't need to know it hits the DB. - -### Further reading - -- [Clean Code, Chapter 2 – Meaningful Names](https://www.oreilly.com/library/view/clean-code-a/9780136083238/) — specifically the section on "Use Intention-Revealing Names" -- [Google TypeScript Style Guide – Naming](https://google.github.io/styleguide/tsguide.html#naming-style) - ---- - -## 2. N+1 query pattern in `validateLanguages` and `logLanguageCoverage` - -### What you wrote - -```ts -for (const language of languages) { - const rows = await db - .selectDistinct({ termId: translations.term_id }) - .from(translations) - .where( - and( - inArray(translations.term_id, termIds), - eq(translations.language_code, language), - ), - ); -} -``` - -### Why it's a problem - -This fires one database query _per language_. If you have 15 supported languages, that's 15 round trips. Each round trip has network latency, connection overhead, and query planning cost. The database already knows how to aggregate across all languages in a single pass — you're just not asking it to. - -This pattern is called **N+1** (one query to get the list, then N queries for each item in the list) and it's one of the most common performance mistakes in applications that use databases. At 15 languages it's fine. At 50 languages with 100k terms, your script will be the reason someone gets paged at 2am. - -### How to fix it - -Ask the database to do the grouping for you in a single query: - -```ts -import { count, ne } from "drizzle-orm"; - -const coverage = await db - .select({ - language: translations.language_code, - coveredCount: count(translations.term_id), - }) - .from(translations) - .where( - and( - inArray(translations.term_id, termIds), - ne(translations.language_code, sourceLanguage), - ), - ) - .groupBy(translations.language_code); - -const validatedLanguages = coverage - .filter((row) => row.coveredCount === termIds.length) - .map((row) => row.language); -``` - -One query. The database returns a row per language with the count of covered terms. You filter in JS. Done. - -### Further reading - -- [Drizzle ORM – `groupBy` and aggregations](https://orm.drizzle.team/docs/select#aggregations) -- ["What is the N+1 query problem" — StackOverflow](https://stackoverflow.com/questions/97197/what-is-the-n1-select-query-problem-and-how-can-it-be-avoided) - ---- - -## 3. Two functions doing the same database work - -### What you wrote - -`validateLanguages` and `logLanguageCoverage` both loop over languages and fire the same query per language. You wrote the same logic twice. - -### Why it's a problem - -This is a violation of **DRY** (Don't Repeat Yourself). The immediate cost is that any bug in the query exists in two places — fixing one doesn't fix the other. The deeper cost is that it doubles your database load for no reason: you fetch the coverage data, use it to compute `validatedLanguages`, throw it away, then fetch it again just to log it. - -### How to fix it - -Once you apply the fix from point 2, you have a single `coverage` array. Use it for both purposes: - -```ts -const coverage = await db... // single query from point 2 - -// Use for validation -const validatedLanguages = coverage - .filter((row) => row.coveredCount === termIds.length) - .map((row) => row.language); - -// Use for logging -for (const row of coverage) { - console.log(` ${row.language}: ${row.coveredCount} / ${termIds.length} terms covered`); -} -``` - -No second trip to the database. - -### Further reading - -- [The DRY Principle](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) - ---- - -## 4. Unnecessary array copying inside a loop - -### What you wrote - -```ts -const wordToTermIds = new Map(); -for (const row of rows) { - const existing = wordToTermIds.get(word) ?? []; - wordToTermIds.set(word, [...existing, row.termId]); // spreads the whole array every iteration -} -``` - -### Why it's a problem - -`[...existing, row.termId]` creates a _brand new array_ every time and copies all the previous elements into it. If "bank" has 3 homonyms, you allocate arrays of size 0, 1, 2, and 3 — throwing the first three away. This is an `O(n²)` memory allocation pattern. For 1000 words it's invisible. In a tighter loop or with more data, it adds up. - -This pattern comes from functional programming habits (immutability is good there). But in a one-off script building a local data structure, there's no reason to avoid mutation. - -### How to fix it - -```ts -const wordToTermIds = new Map(); -for (const row of rows) { - const word = row.text.toLowerCase(); - if (!wordToTermIds.has(word)) { - wordToTermIds.set(word, []); - } - wordToTermIds.get(word)!.push(row.termId); -} -``` - -Get the array once, push into it. No copies. - -### Further reading - -- [MDN – Array.prototype.push()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push) -- [Big O Notation primer](https://www.freecodecamp.org/news/big-o-notation-why-it-matters-and-why-it-doesnt-1674cfa8a23c/) — worth understanding O(n²) vs O(n) - ---- - -## 5. No database transaction — your "idempotent" script can corrupt state - -### What you wrote - -```ts -deckId = await createDeck(validatedLanguages); // step 1 -const addedCount = await addTermsToDeck(deckId, termIds); // step 2 -await updateValidatedLanguages(deckId, validatedLanguages); // step 3 -``` - -### Why it's a problem - -These three operations are separate database round trips with nothing tying them together. If step 2 throws (network blip, constraint violation, anything), you end up with a deck row that has no terms. Run the script again and it finds the existing deck, skips creation, then tries to add terms — but now your `validated_languages` from the previous partial run might be stale. The script _appears_ to recover, but you can't be sure of what state you're in. - -A **transaction** is a guarantee: either all steps succeed together, or none of them do. If anything fails mid-way, the database rolls back to the state before the transaction started. This is fundamental to writing scripts that touch multiple tables. - -### How to fix it - -```ts -await db.transaction(async (tx) => { - const existingDeck = await findExistingDeck(tx); - - let deckId: string; - if (!existingDeck) { - deckId = await createDeck(tx, validatedLanguages); - } else { - deckId = existingDeck.id; - } - - await addTermsToDeck(tx, deckId, termIds); - await updateValidatedLanguages(tx, deckId, validatedLanguages); -}); -``` - -You'll need to thread the `tx` (transaction context) through your functions instead of using the global `db` — that's the key change. - -### Further reading - -- [Drizzle ORM – Transactions](https://orm.drizzle.team/docs/transactions) -- [PostgreSQL – What is a Transaction?](https://www.postgresql.org/docs/current/tutorial-transactions.html) -- [ACID properties explained](https://www.databricks.com/glossary/acid-transactions) — Atomicity is what protects you here - ---- - -## 6. The `isNewDeck` flag is unnecessary - -### What you wrote - -```ts -let isNewDeck: boolean; - -if (!existingDeck) { - deckId = await createDeck(validatedLanguages); - isNewDeck = true; -} else { - deckId = existingDeck.id; - isNewDeck = false; -} - -// ...later... -if (!isNewDeck) { - await updateValidatedLanguages(deckId, validatedLanguages); -} -``` - -### Why it's a problem - -You introduced `isNewDeck` to avoid calling `updateValidatedLanguages` when the deck was just created — reasoning that you already passed `validatedLanguages` to `createDeck`. But that means you're calling `updateValidatedLanguages` in _one path_ and `createDeck(..., validatedLanguages)` in the _other_ path. The intent (always keep validated languages current) is the same in both cases, but the code splits it into two branches you have to mentally reconcile. - -The cleaner model: always call `updateValidatedLanguages` after finding or creating the deck. Then `createDeck` doesn't need `validatedLanguages` at all, and `isNewDeck` disappears. - -### How to fix it - -```ts -const deckId = existingDeck ? existingDeck.id : await createDeck(); // no validatedLanguages needed here - -await addTermsToDeck(deckId, termIds); -await updateValidatedLanguages(deckId, validatedLanguages); // always runs -``` - -Fewer variables, one clear flow. - ---- - -## 7. Comments explain _what_, not _why_ - -### What you wrote - -```ts -// new Set() automatically discards duplicate values, -// and spreading it back with ... converts it to a plain array again. -// So if "bank" appears twice in the file, -// the resulting array will only contain it once. -const words = [ - ...new Set( - raw - .split("\n") - .map((w) => w.trim().toLowerCase()) - .filter(Boolean), - ), -]; -``` - -### Why it's a problem - -Comments that re-explain what the code literally does are called **noise comments**. They add length without adding understanding — any developer who can read this script already knows what `Set` does. Worse, they can get out of date if the code changes but the comment doesn't. - -Good comments explain _why_ a decision was made, not _what_ the code does. The code already says what it does. - -Meanwhile, your most complex line — `const termIds = [...new Set(Array.from(wordToTermIds.values()).flat())]` — has no comment at all. That's the one that earns a note. - -### How to fix it - -```ts -// Deduplicate: multiple words can map to the same term ID (e.g. via synonyms) -const termIds = [...new Set(Array.from(wordToTermIds.values()).flat())]; -``` - -And remove the Set explanation from `readWordlist`. The code is clear. - -### Further reading - -- [Clean Code, Chapter 4 – Comments](https://www.oreilly.com/library/view/clean-code-a/9780136083238/) — specifically "Explain Yourself in Code" and "Noise Comments" - ---- - -## 8. The finished roadmap comment should be deleted - -### What you wrote - -```ts -/* - * roadmap - * [x] Setup - * [x] Read wordlist - * ...all checked off - */ -``` - -### Why it's a problem - -This was useful _while you were planning_. Now that every item is checked, it communicates nothing except "this is done" — which the existence of a working script already communicates. Leaving it in adds noise to the file header and signals that you're not sure what belongs in source control vs. a task tracker. - -### How to fix it - -Delete it. Use GitHub Issues, a Notion doc, or even a scratchpad file for planning notes. Source code is the output of planning, not the place to store it. - ---- - -## 9. No log levels — everything goes to `console.log` - -### What you wrote - -```ts -console.log("📖 Reading word list..."); -console.log(` ${sourceWords.length} words loaded\n`); -// ...and so on for every step -``` - -### Why it's a problem - -In a real environment — CI/CD pipelines, server logs, anything beyond your local terminal — all of this output lands in the same stream at the same priority. Actual errors (`console.error`) get buried in progress logs. There's no way to run the script quietly when you just need the summary, or verbosely when you're debugging. - -For a one-off seed script this is low priority, but it's a habit worth building early. - -### How to fix it - -At minimum, use `console.error` for actual errors (not just in the catch block — also for things like "deck creation returned no ID"). For the detailed per-language breakdown, consider putting it behind a `--verbose` CLI flag so you can run the script cleanly in CI without dumping hundreds of lines of coverage data. - -```ts -// Basic approach -if (process.argv.includes("--verbose")) { - await logLanguageCoverage(termIds); -} -``` - -### Further reading - -- [Node.js `process.argv`](https://nodejs.org/en/learn/command-line/nodejs-accept-arguments-from-the-command-line) -- For a proper solution later: [pino](https://github.com/pinojs/pino) — a lightweight structured logger widely used in Node.js - ---- - -## Summary - -| # | Issue | Priority | -| --- | ------------------------------ | --------------------------------------- | -| 1 | Gerund function names | Low — style, but builds good habits | -| 2 | N+1 queries | High — real performance impact | -| 3 | Duplicate query logic | High — bugs in two places | -| 4 | Array spread in loop | Medium — inefficient pattern to unlearn | -| 5 | No transaction | High — can corrupt database state | -| 6 | `isNewDeck` flag | Low — unnecessary complexity | -| 7 | Comments explain what, not why | Low — style, but important long-term | -| 8 | Roadmap comment left in | Low — cleanup | -| 9 | No log levels | Low — good habit to build | - -Start with **2, 3, and 5** — those are the ones that would cause real problems in production. The rest are about writing code that's easier to read and maintain over time. - -Good luck with the refactor. Come back with the updated script when you're done. diff --git a/documentation/backlog.md b/documentation/backlog.md index 6a91e2d..01db023 100644 --- a/documentation/backlog.md +++ b/documentation/backlog.md @@ -8,15 +8,18 @@ Labels: `[feature]` `[infra]` `[security]` `[ux]` `[debt]` Things that are actively in progress or should be picked up immediately. Mostly operational risk and the remaining phase 7 hardening work. -- **Hetzner domain migration check** `[infra]` - Verify whether the lilastudy.com domain needs to be migrated following a Hetzner DNS change. Check Hetzner dashboard for any pending migration notice. - --- ## next Clearly planned work, not yet started. No hard ordering — sequence based on what unblocks real users first. +- admin dashboard to see users and their status! + +- refactor frontend: play => learn alone, multiplayer => learn together, also multiplayer => public and private games + +- if logged out, navbar should not show play and multiplayer + - **Batch distractor queries to eliminate N+1** `[debt]` createGameSession calls getDistractors once per term in parallel — 3 queries for 3 rounds, 10 for 10. Each query does ORDER BY RANDOM() which can't use an index and gets slower as the translations table grows. Fix: add a getDistractorsForTerms(termIds[], ...) function to @lila/db that batches all distractor fetches into a single query and returns results grouped by term. The service distributes the results per question. Prerequisite: none. Blocked by: nothing, but coordinate with any ongoing @lila/db changes. diff --git a/documentation/design.md b/documentation/design.md deleted file mode 100644 index 4d2f6fa..0000000 --- a/documentation/design.md +++ /dev/null @@ -1,5 +0,0 @@ -# design - -## notes - -break points diff --git a/documentation/notes.md b/documentation/notes.md index 45f8955..bc4c91f 100644 --- a/documentation/notes.md +++ b/documentation/notes.md @@ -115,45 +115,4 @@ Manage your app audience in the Audience page of the Google Auth Platform. - openapi - bruno for api testing - tailscale -- husky/lint-staged - musicforprogramming.net - -## openwordnet - -download libraries via - -```bash -python -c 'import wn; wn.download("omw-fr")'; -``` - -libraries: - -odenet:1.4 -omw-es:1.4 -omw-fr:1.4 -omw-it:1.4 -omw-en:1.4 - -upgrade wn package: - -```bash -pip install --upgrade wn -``` - -check if wn is available, eg italian: - -```bash -python -c "import wn; print(len(wn.words(lang='it', lexicon='omw-it:1.4')))" -``` - -remove a library: - -```bash -python -c "import wn; wn.remove('oewn:2024')"﬌ python -c "import wn; wn.remove('oewn:2024')" -``` - -list all libraries: - -```bash -python -c "import wn; print(wn.lexicons())" -``` diff --git a/documentation/roasts/gameService.md b/documentation/roasts/gameService.md deleted file mode 100644 index 73283de..0000000 --- a/documentation/roasts/gameService.md +++ /dev/null @@ -1,348 +0,0 @@ -# 🔥 GameService Roast: `apps/api/src/services/gameService.ts` - -> *"It works on my machine" is not a scalability strategy.* - -**Project:** lila — Vocabulary Trainer -**File Roasted:** `gameService.ts` -**Date:** $(date) -**Roaster:** Qwen3.6 - ---- - -## 📋 Executive Summary - -| Metric | Score | Notes | -| ------------- | -------- | ---------------------------------------------------- | -| Code Quality | 8/10 | Clean layering, good types, consistent style | -| Correctness | 6/10 | Race condition + N+1 query are critical | -| Test Coverage | 7/10 | Good happy-path tests, missing concurrency tests | -| Scalability | 5/10 | Will choke at ~100 concurrent users without fixes | -| **Overall** | **7/10** | Solid foundation, but fix the footguns before launch | - ---- - -## 🚨 Critical Issues (Fix Before Production) - -### 1. Race Condition: Lost Update in `evaluateAnswer` - -**Location:** `gameService.ts:45-58` + `InMemoryGameSessionStore.ts:update()` - -// Current flow (VULNERABLE): -const session = await store.get(submission.sessionId); // READ -const updatedAnswers = new Map(session.answers); // MODIFY (local copy) -updatedAnswers.delete(submission.questionId); -await store.update(submission.sessionId, { answers: updatedAnswers }); // WRITE - -The Attack: - - Client submits answer A and answer B for the same question (network retry, bug, or malice) - Both requests read the same session.answers Map (question still present) - Both delete the question from their local copy - Both write back → second write overwrites first - Result: One answer is silently lost, session state desyncs - -Why Tests Missed It: Vitest runs tests synchronously. Race conditions require deliberate concurrency testing. -Fix Options: - -// Option A: Add atomic operation to store interface -interface GameSessionStore { - deleteAnswer(sessionId: string, questionId: string): Promise; -} - -// Option B: Use Valkey Lua script for atomic read-modify-write -// Option C: Optimistic locking with version numbers - -Priority: 🔴 CRITICAL — Data integrity issue -2. N+1 Query: Database Performance Bomb -Location: gameService.ts:24-26 + termModel.ts:getDistractors() - -// For each of N terms, we call getDistractors(): -const questions: GameQuestion[] = await Promise.all( - terms.map(async (term) => { - const distractorTexts = await getDistractors(term.termId, ...); // 🚩 N queries! - }) -); - -Impact Analysis: -Rounds - -DB Queries - -At 50 concurrent users -3 - -1 + 3 = 4 - -200 queries/min -10 - -1 + 10 = 11 - -550 queries/min -20 - -1 + 20 = 21 - -1,050 queries/min -Each getDistractors() runs: - -SELECT text FROM terms -JOIN translations ON ... -WHERE pos = $1 AND difficulty = $2 AND term_id != $3 AND text != $4 -ORDER BY RANDOM() LIMIT 6 - -Fix: Batch Fetch Distractors - -// Fetch all distractors in ONE query -const allDistractors = await db - .select({ termId: terms.id, text: translations.text }) - .from(terms) - .innerJoin(translations, /* ... */) - .where(and( - eq(terms.pos, pos), - eq(translations.difficulty, difficulty), - inArray(terms.id, termIds), // Batch! - )) - .limit(DISTRACTOR_FETCH_COUNT * termIds.length); - -// Group by termId in JS, then slice to 3 unique distractors per term -const distractorsByTerm = groupByTermId(allDistractors); - -Priority: 🔴 CRITICAL — Performance/scalability issue - -3. Error Handling Inconsistency -Location: gameService.ts:33-36 - -if (uniqueDistractors.length < 3) { - throw new Error(`Not enough unique distractors for term: ${term.targetText}`); // 🚩 -} - -Problem: Raw Error bypasses your errorHandler middleware: - - No HTTP status mapping (defaults to 500) - No structured logging - Inconsistent API responses - -Fix: -import { UnprocessableEntityError } from "../errors/AppError.js"; - -if (uniqueDistractors.length < 3) { - logger.warn({ termId: term.termId, uniqueCount: uniqueDistractors.length }, - "insufficient_distractors"); - throw new UnprocessableEntityError( - `Not enough unique distractors for term: ${term.targetText}` - ); -} -Priority: 🟡 HIGH — Observability & UX issue -⚠️ High-Severity Smells -4. Code Duplication: Singleplayer vs Multiplayer -Compare: gameService.ts vs multiplayerGameService.ts -// gameService.ts -const optionTexts = [term.targetText, ...uniqueDistractors.slice(0, 3)]; -const shuffledTexts = shuffleArray(optionTexts); -const correctOptionId = shuffledTexts.indexOf(term.targetText); - -// multiplayerGameService.ts (lines 35-45) -const optionTexts = [correctAnswer.targetText, ...distractorTexts]; -const shuffledTexts = shuffle(optionTexts); // Different function, same logic! -const correctOptionId = shuffledTexts.indexOf(correctAnswer.targetText); - -Risks: - - Fix shuffle bias in one place, forget the other - Add new option type (e.g., etymology hint), update one service only - Harder to test core game logic in isolation - -Fix: Extract pure function to @lila/shared or new @lila/game-logic: - -// packages/shared/src/game-logic.ts -export const buildQuestionOptions = ( - correctAnswer: string, - distractors: string[], - optionCount: number = 4 -): { options: AnswerOption[]; correctOptionId: number } => { - const uniqueDistractors = [...new Set(distractors.filter(d => d !== correctAnswer))]; - const optionTexts = [correctAnswer, ...uniqueDistractors.slice(0, optionCount - 1)]; - const shuffled = shuffleSecure(optionTexts); - const correctOptionId = shuffled.indexOf(correctAnswer); - - return { - options: shuffled.map((text, idx) => ({ optionId: idx, text })), - correctOptionId - }; -}; - -Priority: 🟡 HIGH — Maintainability issue -5. Shuffle Bias: Math.random() Trap -Location: utils.ts:shuffleArray() + multiplayerGameService.ts:shuffle() - -export const shuffleArray = (array: T[]): T[] => { - for (let i = result.length - 1; i > 0; i--) { - const j = Math.floor(Math.random() * (i + 1)); // 🚩 Modulo bias + non-crypto RNG - // ... - } -}; - -The Math: - - Math.random() has ~53 bits of entropy (fine for vocab) - Math.floor(rand * n) has modulo bias when n isn't a power of 2 - For n=4: bias is ~0.01% (tiny, but non-zero) - -When It Matters: - - Competitive leaderboards ("option 0 is correct 26% of the time") - Achievement systems based on answer patterns - Security-sensitive features (not applicable here, but principle matters) - -Fix (if needed): -import { randomBytes } from "crypto"; - -const shuffleSecure = (array: T[]): T[] => { - const result = [...array]; - for (let i = result.length - 1; i > 0; i--) { - // Use crypto.getRandomValues for better randomness - const rand = randomBytes(4).readUInt32LE(0); - const j = rand % (i + 1); - [result[i], result[j]] = [result[j], result[i]]; - } - return result; -}; - -Priority: 🟢 LOW — Document tradeoff and move on for now - -6. Test Coverage Gaps -File: gameService.test.ts -✅ Well Tested: - - Happy path: session creation, answer evaluation - Edge cases: duplicate distractors, empty results, invalid inputs - Error propagation from DB layer - -❌ Missing Tests: - -// 1. Concurrency test (race condition) -it("rejects duplicate answers for same question under concurrent load", async () => { - const session = await createGameSession(validRequest, store, "user-1"); - const question = session.questions[0]!; - - // Submit two answers simultaneously - const [result1, result2] = await Promise.allSettled([ - evaluateAnswer({ sessionId, questionId, selectedOptionId: 0 }, store, "user-1"), - evaluateAnswer({ sessionId, questionId, selectedOptionId: 1 }, store, "user-1"), - ]); - - // Exactly one should succeed, one should throw ConflictError - expect([result1, result2].filter(r => r.status === "fulfilled")).toHaveLength(1); -}); - -// 2. TTL expiration test -it("deletes session after TTL expires", async () => { - vi.useFakeTimers(); - const session = await createGameSession(validRequest, store, "user-1"); - - vi.advanceTimersByTime(31 * 60 * 1000); // 31 minutes - - await expect(store.get(session.sessionId)).resolves.toBeNull(); -}); - -// 3. Distractor fallback strategy test -it("uses fallback when <3 unique distractors available", async () => { - mockGetDistractors.mockResolvedValue(["same", "same", "same", "same"]); - // Should either: (a) fetch from broader pool, or (b) reduce rounds gracefully -}); - -Priority: 🟡 HIGH — Prevents regression on critical fixes -🧼 Code Quality Nitpicks -7. Magic Numbers - -// gameService.ts:52 -await store.create(sessionId, {...}, 30 * 60 * 1000); // What is this? - -// termModel.ts:65 -.limit(count); // count=6, but why? - -// shared/schemas/game.ts:15 -optionId: z.number().int().min(0).max(3), // Why 4 options? - -Fix: Centralize in @lila/shared/constants.ts: - -export const GAME_SESSION_TTL_MS = 30 * 60 * 1000; -export const DISTRACTOR_FETCH_COUNT = 6; -export const GAME_OPTION_COUNT = 4; -export const MIN_UNIQUE_DISTRACTORS = 3; - -8. Mutable Reference Leakage -Location: InMemoryGameSessionStore.ts:get() - -get(sessionId: string): Promise { - return Promise.resolve(entry.data); // 🚩 Returns mutable reference to internal state -} - -Risk: Any code that does session.answers.delete(...) mutates the store's internal Map directly. -Fix: - -// Option A: Deep clone (simple, works for this data shape) -return Promise.resolve(structuredClone(entry.data)); - -// Option B: Return readonly view (TypeScript-only protection) -return Promise.resolve(entry.data as Readonly); - -// Option C: Use immutable data structures (overkill for now) - -9. Zero Observability -Problem: No logging, no metrics. You're flying blind in production. -Minimal Fix (5 minutes): - - - -// apps/api/src/lib/logger.ts -import pino from "pino"; -export const logger = pino({ - level: process.env.LOG_LEVEL || "info", - transport: process.env.NODE_ENV === "production" - ? { target: "pino-pretty" } - : undefined -}); - -// In gameService.ts: -import { logger } from "../lib/logger.js"; - -logger.info( - { userId, sourceLang, targetLang, termCount: terms.length }, - "game_session_created" -); - -logger.debug( - { sessionId, questionId, isCorrect, responseTimeMs }, - "answer_evaluated" -); - -Bonus: Export a Prometheus histogram for game_service_duration_seconds. - -10. ORDER BY RANDOM() Time Bomb -Location: termModel.ts:getGameTerms() + getDistractors() - -.orderBy(sql`RANDOM()`) // 🚩 Fine for 10k rows, slow for 1M - -The Comment Admits It: - -// TODO(post-mvp): ORDER BY RANDOM() sorts the entire filtered result set... - -Reality Check: "Post-MVP" never comes without a ticket. -Fix Options: - --- Option A: Pre-computed random_seed column (updated nightly) -WHERE ... AND random_seed >= random() -ORDER BY random_seed -LIMIT $1 - --- Option B: TABLESAMPLE for approximate sampling (Postgres 9.5+) -FROM terms TABLESAMPLE SYSTEM(10) -WHERE ... -LIMIT $1 - --- Option C: Random offset (simple, but still scans) -OFFSET floor(random() * (SELECT count(*) FROM terms WHERE ...)) - -Action: Add a ticket to documentation/tickets/t00009.md now. \ No newline at end of file diff --git a/documentation/tickets/blueprint.md b/documentation/tickets/blueprint.md deleted file mode 100644 index c5fdf8a..0000000 --- a/documentation/tickets/blueprint.md +++ /dev/null @@ -1,95 +0,0 @@ -# Ticket Blueprint - -Two formats depending on task type. Choose based on whether a meaningful -decision between options was made. - ---- - -## Format A — ADR (architectural/infrastructural decisions) - -Use when: you chose between options with long-term consequences. -Prefix: `adr-` - ---- - -# ADR: - -## Status - -Accepted | Superseded by | Deprecated - -## Date - -YYYY-MM-DD - -## Context - -What is the problem? Why does it need to be solved? - -## Decision - -What was chosen and why in one or two sentences. - -## Options considered - -### Option A — <name> ✅ - -Description. Why it was chosen. - -### Option B — <name> - -Description. Why it was rejected. - -## Consequences - -- What gets better -- What gets worse or more complex -- Operational implications -- What breaks if this needs to be redone - -## Affected files / machines - -- List files, servers, or systems touched - -## References - -- Links to relevant docs - ---- - -## Setup guide / implementation notes - -Step-by-step of what was actually done. - ---- - -## Format B — Task (features, fixes, chores) - -Use when: routine task with a clear solution. -Prefix: `feat-` / `fix-` / `chore-` - ---- - -# <prefix>: <title> - -## Problem - -What was wrong or missing? - -## Options considered - -### Option A — <name> ✅ - -### Option B — <name> - -## Solution - -What was done and why. - -## Files changed - -- `path/to/file.ts` - -## Commit - -`<type>: <message>` diff --git a/documentation/tickets/t00001.md b/documentation/tickets/t00001.md deleted file mode 100644 index f7f1a09..0000000 --- a/documentation/tickets/t00001.md +++ /dev/null @@ -1,107 +0,0 @@ -# ADR: Docker Credential Helper Setup - -## Status - -Accepted - -## Date - -2026-04-26 - -## Context - -Docker credentials for `git.lilastudy.com` and `dhi.io` were stored as base64-encoded strings in `~/.docker/config.json` on both the dev laptop and the VPS. Base64 is not encryption — anyone with read access to the file can decode the credentials instantly. - -## Decision - -Use `pass` (GPG-backed password store) as the Docker credential helper on both machines. - -## Options considered - -### Option A — `pass` (GPG-backed) ✅ - -Stores credentials encrypted with a GPG key. Works on headless servers and desktops without GNOME. Industry standard for Linux servers. - -### Option B — `secretservice` (GNOME keyring) - -Uses the desktop keyring daemon. Not suitable for a headless VPS, and not suitable for an i3 desktop without running `gnome-keyring-daemon` manually. - -### Option C — `gnome-libsecret` - -Same limitations as Option B. - -## Consequences - -- Credentials are now GPG-encrypted at rest on both machines -- Requires GPG passphrase entry when Docker needs to pull credentials - in a new session -- Must be set up manually on each machine — not reproducible via the repo -- VPS setup must be repeated if the server is reprovisioned - -## Affected machines - -- Dev laptop (Debian 13, i3) -- VPS (Debian 13, ARM64, headless) - -## References - -- [docker docs](https://docs.docker.com/reference/cli/docker/login/#credential-stores) -- [pass docs](https://www.passwordstore.org/) - ---- - -## Setup guide - -Repeat these steps on each machine. - -### 1. Install dependencies - -```bash -sudo apt-get install -y pass gnupg2 golang-docker-credential-helpers -``` - -### 2. Generate a GPG key - -```bash -gpg --full-generate-key -``` - -Choose RSA, 4096 bits, no expiry. Set a strong passphrase. - -### 3. Get the key ID - -```bash -gpg --list-secret-keys --keyid-format LONG -``` - -Copy the hex string after the `/` on the `sec` line. - -### 4. Initialise pass - -```bash -pass init <your-key-id> -``` - -### 5. Update `~/.docker/config.json` - -Replace the entire file contents with: - -```json -{ "credsStore": "pass" } -``` - -### 6. Re-login to registries - -```bash -docker login git.lilastudy.com -# dev laptop only: -docker login dhi.io -``` - -### 7. Verify - -```bash -cat ~/.docker/config.json -``` - -Should show only `"credsStore": "pass"` with no `auths` block. diff --git a/documentation/tickets/t00002.md b/documentation/tickets/t00002.md deleted file mode 100644 index 4f7fbb5..0000000 --- a/documentation/tickets/t00002.md +++ /dev/null @@ -1,149 +0,0 @@ -# ADR: Change GAME_ROUNDS from strings to numbers - -## Status - -Accepted - -## Date - -2026-04-28 - -## Context - -`GAME_ROUNDS` in `packages/shared/src/constants.ts` was typed as `["3", "10"] as const`, making `GameRounds` a string union (`"3" | "10"`). This meant `gameService.ts` had to cast the value with `Number(request.rounds)` deep in business logic — a type conversion happening far from the boundary where data enters the system. The type system was lying: `rounds` was described as a string everywhere but used as a number where it mattered. - -## Decision - -Change `GAME_ROUNDS` to `[3, 10] as const` and update the Zod schema to use `z.literal(GAME_ROUNDS)` instead of `z.enum(GAME_ROUNDS)`. The single source of truth remains `constants.ts` — adding a new round count (e.g. `20`) requires only editing that file. - -## Options considered - -### Option A — Numbers everywhere ✅ - -Change `GAME_ROUNDS` to `[3, 10] as const`. Use `z.literal(GAME_ROUNDS)` in the schema. Update the frontend component state and `SettingGroup` props. Drop `Number()` cast in the service. - -Chosen because: JSON carries numbers natively, both ends of the wire are owned by this codebase, and type conversions belong at the boundary — not inside business logic. - -### Option B — Keep strings, accept the cast - -Leave `GAME_ROUNDS` as `["3", "10"]`. The `Number()` cast stays in `gameService.ts`. - -Rejected because: it pushes type conversion into business logic and makes the inferred `GameRequest` type misleading. The cast has to live somewhere — the schema boundary is the right place. - -### Option C — Coerce at the schema boundary - -Keep `GAME_ROUNDS` as numbers but use `z.coerce.number().pipe(z.literal(GAME_ROUNDS))` so the frontend can keep sending strings. - -Rejected because: coercion is for untrusted or uncontrolled inputs (form fields, query params, third-party clients). We control both ends of the wire. Coercing a self-inflicted type mismatch is treating a wound we gave ourselves. - -## Consequences - -- `GameRounds` is now `3 | 10` instead of `"3" | "10"` -- `Number(request.rounds)` cast removed from `gameService.ts` -- `SettingGroup` in `GameSetup.tsx` now accepts `string | number` options -- `useState<string>` for rounds changed to `useState<number>` -- Adding a new round count requires only editing `GAME_ROUNDS` in `constants.ts` -- `z.enum` cannot be used for number literals — `z.literal` must be used instead (this is a Zod constraint, not a project convention) - -## Affected files - -- `packages/shared/src/constants.ts` -- `packages/shared/src/schemas/game.ts` -- `apps/api/src/services/gameService.ts` -- `apps/api/src/services/gameService.test.ts` -- `apps/api/src/controllers/gameController.test.ts` -- `apps/web/src/components/game/GameSetup.tsx` - -## References - -- [Zod literals](https://zod.dev/?id=literals) - ---- - -## Setup guide / implementation notes - -1. In `packages/shared/src/constants.ts`, change: - - ```ts - export const GAME_ROUNDS = ["3", "10"] as const; - ``` - - to: - - ```ts - export const GAME_ROUNDS = [3, 10] as const; - ``` - -2. In `packages/shared/src/schemas/game.ts`, change: - - ```ts - rounds: z.enum(GAME_ROUNDS), - ``` - - to: - - ```ts - rounds: z.literal(GAME_ROUNDS), - ``` - -3. In `apps/api/src/services/gameService.ts`, change: - - ```ts - Number(request.rounds), - ``` - - to: - - ```ts - request.rounds, - ``` - -4. In `apps/api/src/services/gameService.test.ts`, change: - - ```ts - rounds: "3", - ``` - - to: - - ```ts - rounds: 3, - ``` - -5. In `apps/api/src/controllers/gameController.test.ts`, change: - - ```ts - rounds: "3", - ``` - - to: - - ```ts - rounds: 3, - ``` - - Also add a pinning test before the refactor: - - ```ts - it("returns 400 when rounds has an invalid value", async () => { - const res = await request(app) - .post("/api/v1/game/start") - .send({ ...validBody, rounds: "invalid" }); - expect(res.status).toBe(400); - expect(res.body.success).toBe(false); - }); - ``` - -6. In `apps/web/src/components/game/GameSetup.tsx`: - - Update `SettingGroup` props to accept `string | number`: - - ```ts - type SettingGroupProps = { - options: readonly (string | number)[]; - selected: string | number; - onSelect: (value: string | number) => void; - }; - ``` - - - Update `LABELS` lookup to `LABELS[String(option)]` - - Change rounds state from `useState<string>` to `useState<number>` diff --git a/documentation/tickets/t00003.md b/documentation/tickets/t00003.md deleted file mode 100644 index 774fe42..0000000 --- a/documentation/tickets/t00003.md +++ /dev/null @@ -1,37 +0,0 @@ -# refactor: extract shuffleArray to lib/utils, rename correctAnswers to terms - -## Problem - -Two readability issues in `gameService.ts`: - -1. `shuffle` was defined as a private function at the bottom of `gameService.ts`, after the function that calls it. It is a pure generic utility with no dependency on game domain logic, so it had no business living there. - -2. The variable holding terms fetched from the database was named `correctAnswers`. These are word pairs — they only become "correct answers" once options are built around them. The name was premature and misleading. - -## Options considered - -### Option A — Move `shuffle` up in the same file - -Simple, no new files. Fixes the ordering issue but keeps a generic utility buried in domain code. - -### Option B — Extract to `lib/utils.ts` ✅ - -Move `shuffle` (renamed `shuffleArray`) to `apps/api/src/lib/utils.ts` and import it. Cleaner separation: domain logic stays in services, generic utilities live in `lib/`. - -Chosen because `lib/` already exists, the function is reusable, and it gives future utilities a home. - -## Solution - -- Created `apps/api/src/lib/utils.ts` with `shuffleArray` -- Renamed `shuffle` → `shuffleArray` for clarity at the call site -- Removed the inline `shuffle` from `gameService.ts` and imported from `lib/utils.ts` -- Renamed `correctAnswers` → `terms` and `correctAnswer` → `term` throughout `gameService.ts` - -## Files changed - -- `apps/api/src/lib/utils.ts` — created -- `apps/api/src/services/gameService.ts` — removed `shuffle`, updated import, renamed variables - -## Commit - -`refactor: extract shuffleArray to lib/utils, rename correctAnswers to terms` diff --git a/documentation/tickets/t00004.md b/documentation/tickets/t00004.md deleted file mode 100644 index f7d5fea..0000000 --- a/documentation/tickets/t00004.md +++ /dev/null @@ -1,110 +0,0 @@ -# ADR: Dependency injection for GameSessionStore via composition root - -## Status - -Accepted - -## Date - -2026-04-28 - -## Context - -`gameService.ts` had a module-level singleton: - -```ts -const gameSessionStore = new InMemoryGameSessionStore(); -``` - -This made the store invisible to anything outside the file. The `GameSessionStore` interface existed to make the store swappable — but the singleton made that impossible without editing the service itself. Tests shared the same instance across every test run, creating the potential for ghost sessions leaking between tests. The controller also briefly owned the singleton during an intermediate step, which violated the principle that controllers should only handle HTTP concerns. - -## Decision - -Adopt a composition root pattern. The store is created once in `createApp()` and passed down through factory functions: `createApiRouter(store)` → `createGameRouter(store)` → `createGameController(store)` → service calls. Neither the controller nor the service knows which implementation they're working with — they both see `GameSessionStore`. - -## Options considered - -### Option A — Composition root ✅ - -Convert routers and controllers to factory functions. Create the store in `createApp()` and pass it down. The store is created once, at the top, and injected through the call chain. - -Chosen because: clean separation of concerns, no layer below `createApp()` needs to know the concrete implementation, swapping to `ValKeyGameSessionStore` is a one-line change in `app.ts`, and tests get fresh isolated store instances. - -### Option B — Keep singleton in controller - -Leave the store as a module-level singleton in `gameController.ts`. Controllers own the store lifetime. - -Rejected because: controllers should only handle HTTP concerns. Owning infrastructure lifetime is not an HTTP concern. - -### Option C — DI framework (tsyringe, inversify) - -Use a proper dependency injection container. - -Rejected because: overkill for the current scale. The composition root pattern achieves the same result with zero dependencies and no magic. - -## Consequences - -- Swapping `InMemoryGameSessionStore` for `ValKeyGameSessionStore` requires editing one line in `app.ts` -- Tests create fresh `InMemoryGameSessionStore` instances per test — no shared state, no ghost sessions -- Routers and controllers are now factory functions instead of module-level singletons — slightly more verbose but explicitly testable -- `gameController.test.ts` uses `createApp()` which owns the store — controller tests remain integration-style and unaffected -- All layers below `createApp()` depend only on the `GameSessionStore` interface, never the concrete implementation - -## Affected files - -- `apps/api/src/app.ts` — creates the store, passes to `createApiRouter` -- `apps/api/src/routes/apiRouter.ts` — converted to `createApiRouter(store)` factory -- `apps/api/src/routes/gameRouter.ts` — converted to `createGameRouter(store)` factory -- `apps/api/src/controllers/gameController.ts` — converted to `createGameController(store)` factory -- `apps/api/src/services/gameService.ts` — `store` parameter added to both functions, singleton removed -- `apps/api/src/services/gameService.test.ts` — fresh store per describe block via `beforeEach` - -## References - -- [Composition root pattern](https://blog.ploeh.dk/2011/07/28/CompositionRoot/) - ---- - -## Setup guide / implementation notes - -1. `gameService.ts` — remove module-level singleton, add `store: GameSessionStore` parameter to `createGameSession` and `evaluateAnswer` - -2. `gameController.ts` — convert exported functions to a factory: - - ```ts - export const createGameController = (store: GameSessionStore) => ({ - createGame: async (req, res, next) => { ... }, - submitAnswer: async (req, res, next) => { ... }, - }); - ``` - -3. `gameRouter.ts` — convert to factory: - - ```ts - export const createGameRouter = (store: GameSessionStore): Router => { - const router = express.Router(); - const controller = createGameController(store); - router.post("/start", controller.createGame); - router.post("/answer", controller.submitAnswer); - return router; - }; - ``` - -4. `apiRouter.ts` — convert to factory: - - ```ts - export const createApiRouter = (store: GameSessionStore): Router => { - const router = express.Router(); - router.use("/game", createGameRouter(store)); - return router; - }; - ``` - -5. `app.ts` — create the store at the composition root: - - ```ts - const store = new InMemoryGameSessionStore(); - app.use("/api/v1", createApiRouter(store)); - ``` - -6. `gameService.test.ts` — add `let store: InMemoryGameSessionStore` to each `describe` block, reset in `beforeEach`, pass to every service call diff --git a/documentation/tickets/t00005.md b/documentation/tickets/t00005.md deleted file mode 100644 index baf5e2b..0000000 --- a/documentation/tickets/t00005.md +++ /dev/null @@ -1,93 +0,0 @@ -# ADR: Session lifecycle — TTL and replay protection - -## Status - -Accepted - -## Date - -2026-04-28 - -## Context - -`InMemoryGameSessionStore` had no TTL and no cleanup mechanism. Every session created stayed in memory until the process restarted. Additionally, `evaluateAnswer` never removed a question from the answer key after evaluating it, meaning the same question could be submitted multiple times and receive a valid result each time — a potential exploit in multiplayer and a correctness bug in singleplayer. - -## Decision - -Add a `ttlMs` parameter to `GameSessionStore.create()` so both the in-memory and future Valkey implementations handle expiry consistently. Delete questions from the answer key after evaluation. Delete the session when the last question is answered. - -## Options considered - -### Option A — Delete on last answer only - -Simple. Covers replay protection and normal session completion. Abandoned sessions (player starts game, never finishes) still leak memory. - -### Option B — Delete on last answer + TTL on the interface ✅ - -Delete on answer covers normal flow. TTL covers abandoned sessions. TTL on the interface means `ValKeyGameSessionStore` can use Redis-native `EXPIRE` without any interface changes during migration. - -Chosen because it closes the memory leak entirely and makes the Valkey migration a zero-interface-change operation. - -### Option C — TTL hardcoded inside InMemoryGameSessionStore only - -Simpler short-term. But the interface wouldn't carry the TTL parameter, so `ValKeyGameSessionStore` would need a different mechanism — inconsistency between implementations. - -## Consequences - -- Sessions expire after 30 minutes of inactivity regardless of completion state -- Submitting the same question twice throws `NotFoundError` on the second attempt -- Sessions are deleted automatically when the last question is answered -- `GameSessionStore.create()` now requires a `ttlMs` argument — any future implementation must honour it -- `ValKeyGameSessionStore` can implement TTL via Redis `EXPIRE` with no interface changes -- `InMemoryGameSessionStore` stores `{ data, expiresAt }` entries instead of raw `GameSessionData` — expiry is checked lazily on `get()` - -## Affected files - -- `apps/api/src/gameSessionStore/GameSessionStore.ts` — `ttlMs` added to `create` -- `apps/api/src/gameSessionStore/InMemoryGameSessionStore.ts` — TTL implementation -- `apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts` — new test file -- `apps/api/src/services/gameService.ts` — passes TTL to `store.create`, deletes question after evaluation, deletes session when empty -- `apps/api/src/services/gameService.test.ts` — replay protection and session cleanup tests added - -## References - -- [Redis EXPIRE command](https://redis.io/commands/expire/) - ---- - -## Setup guide / implementation notes - -1. `GameSessionStore.ts` — add `ttlMs` to `create`: - - ```ts - create(sessionId: string, data: GameSessionData, ttlMs: number): Promise<void>; - ``` - -2. `InMemoryGameSessionStore.ts` — wrap stored data with expiry: - - ```ts - type SessionEntry = { data: GameSessionData; expiresAt: number }; - ``` - - Check expiry on `get()`, delete expired entries lazily. - -3. `gameService.ts` — pass TTL when creating session: - - ```ts - await store.create(sessionId, { answers: answerKey }, 30 * 60 * 1000); - ``` - - After evaluating an answer: - - ```ts - session.answers.delete(submission.questionId); - if (session.answers.size === 0) { - await store.delete(submission.sessionId); - } - ``` - -4. When implementing `ValKeyGameSessionStore`, pass `ttlMs` to Redis `EXPIRE`: - - ```ts - await valkey.set(sessionId, serialize(data), "EX", Math.ceil(ttlMs / 1000)); - ``` diff --git a/documentation/tickets/t00006.md b/documentation/tickets/t00006.md deleted file mode 100644 index 8e777f0..0000000 --- a/documentation/tickets/t00006.md +++ /dev/null @@ -1,125 +0,0 @@ -# ADR: Session ownership check and AuthenticatedRequest type - -## Status - -Accepted - -## Date - -2026-04-28 - -## Context - -`evaluateAnswer` accepted any `sessionId` without verifying it belonged to the requesting user. The only protection was the unguessability of a UUID — security through obscurity. If a user intercepted or guessed another user's `sessionId`, they could submit answers on their behalf. - -Additionally, protected controller handlers typed their `req` parameter as `Request`, making `session` optional even though `requireAuth` middleware guarantees it is present. This required non-null assertions (`req.session!`) in business logic — a type assertion that could cause a runtime crash if middleware ordering ever changed. - -## Decision - -Store `userId` in `GameSessionData`. Pass `userId` from the controller into both `createGameSession` and `evaluateAnswer`. Assert ownership on evaluation — if the session's `userId` doesn't match the requesting user's ID, throw `NotFoundError`. Introduce `AuthenticatedRequest` to eliminate non-null assertions in protected handlers. - -## Options considered - -### Option A — AuthenticatedRequest type ✅ - -Define `AuthenticatedRequest = Request & { session: { session: Session; user: User } }` in `types/express.d.ts`. Use it in protected controller handlers instead of `Request`. Requires a single `as express.RequestHandler` cast at route registration due to Express's type limitations. - -Chosen because: eliminates dangerous non-null assertions in business logic. The cast at route registration is a necessary cast caused by a third-party library limitation, not uncertain logic. - -### Option B — Non-null assertion (`req.session!`) - -Keep `Request` on all handlers. Assert `req.session!` at every usage. - -Rejected because: non-null assertions in business logic are dangerous — if middleware ordering ever changes, the assertion silently passes and crashes at runtime. - ---- - -### Option C — NotFoundError (404) on ownership failure ✅ - -When a session exists but belongs to a different user, throw `NotFoundError` with the same message as a missing session. - -Chosen because: session IDs are opaque secrets. Returning 403 would confirm to the caller that the session ID is valid and belongs to someone else — information they shouldn't have. This pattern is used by GitHub, AWS, and most security-conscious APIs. - -### Option D — ForbiddenError (403) on ownership failure - -Explicit error that distinguishes "not found" from "not allowed". - -Rejected because: for user-owned resources identified by opaque IDs, confirming existence to an unauthorised caller is an information leak. 404 is the industry standard for this case. - -## Consequences - -- Alice cannot submit answers for Bob's session — ownership is verified at the service layer -- `req.session.user.id` is accessible without non-null assertions in protected handlers -- `GameSessionData` now carries `userId` — any future `GameSessionStore` implementation must store and return it -- Route registration requires `as express.RequestHandler` cast for protected handlers — one cast per route, in wiring code only -- `ValKeyGameSessionStore` must serialise and deserialise `userId` alongside `answers` - -## Affected files - -- `apps/api/src/types/express.d.ts` — `AuthenticatedRequest` type added -- `apps/api/src/gameSessionStore/GameSessionStore.ts` — `userId` added to `GameSessionData` -- `apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts` — updated data fixtures -- `apps/api/src/services/gameService.ts` — `userId` parameter added to both functions, ownership assertion in `evaluateAnswer` -- `apps/api/src/services/gameService.test.ts` — updated all calls, ownership test added -- `apps/api/src/controllers/gameController.ts` — extracts `userId` from `req.session.user.id`, passes to service calls -- `apps/api/src/routes/gameRouter.ts` — `as express.RequestHandler` cast at route registration - -## References - -- [OWASP: Insecure Direct Object Reference](https://owasp.org/www-community/attacks/Insecure_Direct_Object_Reference) -- [HTTP 403 vs 404 for authorization failures](https://stackoverflow.com/questions/3297048/403-forbidden-vs-401-unauthorized-http-responses) - ---- - -## Setup guide / implementation notes - -1. `types/express.d.ts` — add: - - ```ts - export type AuthenticatedRequest = Request & { - session: { session: Session; user: User }; - }; - ``` - -2. `GameSessionStore.ts` — add `userId` to `GameSessionData`: - - ```ts - export type GameSessionData = { answers: Map<string, number>; userId: string }; - ``` - -3. `gameService.ts` — add `userId` to both function signatures: - - ```ts - export const createGameSession = async ( - request: GameRequest, - store: GameSessionStore, - userId: string, - ): Promise<GameSession> - ``` - - Store it on create: - - ```ts - await store.create(sessionId, { answers: answerKey, userId }, 30 * 60 * 1000); - ``` - - Assert on evaluate: - - ```ts - if (!session || session.userId !== userId) { - throw new NotFoundError(`Game session not found: ${submission.sessionId}`); - } - ``` - -4. `gameController.ts` — extract from authenticated request: - - ```ts - req.session.user.id - ``` - -5. `gameRouter.ts` — cast at registration: - - ```ts - router.post("/start", controller.createGame as express.RequestHandler); - router.post("/answer", controller.submitAnswer as express.RequestHandler); - ``` diff --git a/documentation/tickets/t00007.md b/documentation/tickets/t00007.md deleted file mode 100644 index 5469049..0000000 --- a/documentation/tickets/t00007.md +++ /dev/null @@ -1,41 +0,0 @@ -# feat: guard against empty terms in createGameSession - -## Problem - -If `getGameTerms` returned an empty array — no vocabulary data matched the requested language, difficulty, and part of speech combination — `createGameSession` would create a session with zero questions and return it. The frontend would receive an empty `questions` array, attempt to render the first question, find nothing, and crash with no useful error message shown to the user. - -## Options considered - -### Option A — `NotFoundError` (404) ✅ - -Throw when `terms.length === 0` before any session is created. The combination of filters yielded no data — that's a "not found" situation. - -Chosen because: the request is technically valid (all filter values are recognised), but the combination has no matching data. 404 is the correct semantic response. - -### Option B — `ValidationError` (400) - -Treat empty results as a bad request. - -Rejected because: the client sent valid input. The problem is missing data, not invalid input. 400 would be misleading. - -## Solution - -Added a guard in `createGameSession` immediately after `getGameTerms`: - -```ts -if (terms.length === 0) { - throw new NotFoundError("No terms found for the given filters"); -} -``` - -The error propagates through the controller's `try/catch` to the error handler, which returns a clean 404 response. No session is created. - -## Files changed - -- `apps/api/src/services/gameService.ts` — empty terms guard added -- `apps/api/src/services/gameService.test.ts` — pinning test added -- `apps/api/src/controllers/gameController.test.ts` — pinning test added at HTTP layer - -## Commit - -`feat: guard against empty terms in createGameSession` diff --git a/documentation/tickets/t00008.md b/documentation/tickets/t00008.md deleted file mode 100644 index b171abc..0000000 --- a/documentation/tickets/t00008.md +++ /dev/null @@ -1,54 +0,0 @@ -# fix: deduplicate distractors, replace tautological test - -## Problem - -Two issues in `createGameSession` and its test suite: - -1. If `getDistractors` returned the correct answer as one of the distractors, `createGameSession` would include it in the options array without filtering it out. `indexOf` would then find the first occurrence, which might not be the one intended as the correct answer — producing a question where the correct answer appears twice and the stored `correctOptionId` is wrong. - -2. The test `"distractors are never the correct answer"` was tautological — it filtered the correct answer out of the options array, then asserted the remaining items were not the correct answer. It was testing that `Array.filter()` works. It could never fail. - -## Options considered - -### Option A — Filter duplicates after fetching, request extra distractors as buffer ✅ - -Filter out any distractor that matches the correct answer after fetching. Request 6 distractors instead of 3 to ensure enough remain after deduplication. Take the first 3 valid ones with `slice(0, 3)`. - -Chosen because: deduplication at the service layer is the right place — `getDistractors` shouldn't need to know what the correct answer is. Requesting extra provides a buffer against collisions. - -### Option B — Fix `getDistractors` to never return the correct answer - -Add a NOT filter in the database query. - -Not chosen for this ticket — the database query is in `@lila/db` and is a separate concern. The service layer should be defensive regardless of what the model layer returns. - -## Solution - -- Filter distractors against the correct answer before building options: - - ```ts - const uniqueDistractors = distractorTexts.filter((t) => t !== term.targetText); - const optionTexts = [term.targetText, ...uniqueDistractors.slice(0, 3)]; - ``` - -- Request 6 distractors instead of 3 to account for potential duplicates -- Replaced tautological test with a test that actually exercises the duplicate case: - - ```ts - it("correct answer appears exactly once even if getDistractors returns a duplicate", ...) - ``` - -- Added distractor failure propagation test: - - ```ts - it("propagates getDistractors failure", ...) - ``` - -## Files changed - -- `apps/api/src/services/gameService.ts` — deduplication logic, distractor count increased to 6 -- `apps/api/src/services/gameService.test.ts` — tautological test replaced, failure test added - -## Commit - -`fix: deduplicate distractors, replace tautological test, add distractor failure test`