Uh oh!
There was an error while loading. Please reload this page.
chore(db): timestamp-prefix new migrations; freeze the legacy NNNN_ set - #509
Merged
Conversation
Sequential migration numbers are claimed when a branch is WRITTEN but only validated when it MERGES, so concurrent branches routinely pick the same one. PR #507 hit this twice in a single branch lifetime: first against main's 0042, then against an unpushed branch already holding 0043/0044 — invisible on GitHub, and only found because both had been applied to the same local database. New migrations now use a UTC timestamp prefix (YYYYMMDDHHMMSS_description.sql, `date -u +%Y%m%d%H%M%S`). There is no shared counter, so two branches would have to be created in the same second to collide. THE 45 EXISTING FILES ARE NOT RENAMED, AND MUST NEVER BE. `schema_migrations.filename` is the ledger's primary key and `pending_migrations` treats an unrecorded basename as unapplied, so renaming an applied migration makes the runner apply it AGAIN. 0021_gradebook.sql DROPs and re-CREATEs the assignments table — a bulk rename would destroy the gradebook on every environment that has already run it. The two conventions coexist permanently. Ordering holds, but for a narrower reason than "timestamps are longer": comparison is character-by-character, so length decides nothing — a year-1000 timestamp would sort BEFORE a 9999_ prefix. What actually holds is that every legacy file starts with "0" and every timestamp this millennium starts with "2". A test pins that reason, counter-example included, so the next reader does not re-derive the wrong one. (An initial version of this change asserted the length-based claim; its own boundary test falsified it.) Enforcement is a test, not a note: test_migration_naming.py fails if a new NNNN_ file appears. The existing prefix test in test_migrations.py had to be relaxed to accept both shapes — it would otherwise reject every timestamped migration. Full suite: 1542 passed, 38 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughMigration discovery now accepts legacy four-digit and UTC timestamp-prefixed filenames. Documentation defines the naming transition and migration rules. Tests validate filename formats, ordering, and preservation of existing numbered migrations. ChangesMigration naming
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 294eabb | Commit Preview URL Branch Preview URL | Aug 01 2026, 02:48 AM |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrate.py`:
- Around line 42-51: The migration-name validation in is_valid_migration_name
must restrict 14-digit timestamp prefixes to ASCII timestamps beginning with 2,
while preserving existing 4-digit legacy prefixes; update _MIGRATION_NAME_RE in
backend/db/migrate.py (lines 42-51). Add an assertion that 10000101000000_x.sql
is rejected in backend/tests/test_migration_naming.py (lines 74-89).
In `@backend/tests/test_migration_naming.py`:
- Around line 100-106: Update
test_no_sequential_migration_has_been_added_since_the_cutover to assert that the
discovered legacy migration basenames exactly match a frozen expected set, not
only that their count equals _LEGACY_NUMERIC_COUNT. Preserve the existing
failure message context and ensure renames, additions, or removals of legacy
NNNN_ files fail the test.
In `@CLAUDE.md`:
- Line 83: Update the migration creation guidance near the existing “new
numbered file” instruction to require a UTC timestamp-prefixed filename using
the established `date -u +%Y%m%d%H%M%S` convention. Keep the guidance that
existing sequential migrations are frozen and must not be renamed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 44443bbd-d299-4a34-b7c6-f49168fd053a
📒 Files selected for processing (5)
CLAUDE.mdbackend/db/migrate.pybackend/db/migrations/README.mdbackend/tests/test_migration_naming.pybackend/tests/test_migrations.py
Comment on lines
+42
to
+51
| _MIGRATION_NAME_RE = re.compile(r"^(\d{4}|\d{14})_.+\.sql$") | ||
| def is_valid_migration_name(name: str) -> bool: | ||
| """True when a filename carries a sortable, fixed-width numeric prefix. | ||
| Anything else sorts unpredictably against its siblings, which silently | ||
| changes apply order. | ||
| """ | ||
| return bool(_MIGRATION_NAME_RE.match(name)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -n -C 3 'MIGRATION_NAME_RE|10000101000000' \
backend/db/migrate.py backend/tests/test_migration_naming.pyRepository: SaplingLearn/Sapling
Length of output: 1632
🏁 Script executed:
#!/bin/bashset -euo pipefail
sed -n '30,120p' backend/db/migrate.py
printf'\n--- tests ---\n'
sed -n '50,105p' backend/tests/test_migration_naming.py
printf'\n--- deterministic string/order probe ---\n'
python3 - <<'PY'NAME_RE = __import__('re').compile(r"^(\d{4}|\d{14})_.+\.sql$")def is_valid(name): return bool(NAME_RE.match(name))legacy_names = ["0001_x.sql", "0042_x.sql", "0999_x.sql"]new_names = [ "20260731224500_x.sql", "10000101000000_x.sql", "09999999999999_x.sql",]def run_order(names): return sorted([n for n in names if is_valid(n)])print("10000101000000_x.sql valid?", is_valid("10000101000000_x.sql"))print("sorted with legacy?", run_order(legacy_names + ["10000101000000_x.sql", "20260731224500_x.sql"]))PYRepository: SaplingLearn/Sapling
Length of output: 6497
Enforce the timestamp ordering domain.
is_valid_migration_name() accepts 10000101000000_x.sql, but that prefix sorts before legacy 0NNN_ migrations. Because filename order is apply order, a new migration can run before the legacy schema unless invalid prefixes are rejected.
backend/db/migrate.py#L42-L51: restrict new 14-digit prefixes to ASCII2-prefixed timestamps.backend/tests/test_migration_naming.py: assert that10000101000000_x.sqlis invalid.
📍 Affects 2 files
backend/db/migrate.py#L42-L51(this comment)backend/tests/test_migration_naming.py#L74-L89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/db/migrate.py` around lines 42 - 51, The migration-name validation in
is_valid_migration_name must restrict 14-digit timestamp prefixes to ASCII
timestamps beginning with 2, while preserving existing 4-digit legacy prefixes;
update _MIGRATION_NAME_RE in backend/db/migrate.py (lines 42-51). Add an
assertion that 10000101000000_x.sql is rejected in
backend/tests/test_migration_naming.py (lines 74-89).
Comment on lines
+100
to
+106
| def test_no_sequential_migration_has_been_added_since_the_cutover(self): | ||
| legacy = [n for n in _names() if re.match(r"^\d{4}_", n)] | ||
| assert len(legacy) == _LEGACY_NUMERIC_COUNT, ( | ||
| f"expected {_LEGACY_NUMERIC_COUNT} legacy NNNN_ migrations, found " | ||
| f"{len(legacy)}. New migrations must use a UTC timestamp prefix " | ||
| "(YYYYMMDDHHMMSS_description.sql) — see this module's docstring." | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
ast-grep outline backend/tests/test_migrations.py --items all --type function
rg -n -C 4 'legacy.*(set|name)|0021_gradebook|schema_migrations|expected.*migration' \
backend/tests backend/dbRepository: SaplingLearn/Sapling
Length of output: 19094
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"## backend/tests/test_migration_naming.py first 120 lines"
cat -n backend/tests/test_migration_naming.py | sed -n '1,130p'echoecho"## backend/tests/test_migrations.py"
cat -n backend/tests/test_migrations.py | sed -n '1,120p'echoecho"## backend/tests/integration/test_migrations_ledger.py"
cat -n backend/tests/integration/test_migrations_ledger.py | sed -n '1,120p'echoecho"## migration file basenames"
find backend/db/migrations -maxdepth 1 -type f -name '*.sql'| sed 's#^`#backend/`#'| xargs -r -n1 basename | sort | sed -n '1,80p'echoecho"## exact pinned legacy/count references"
rg -n '_LEGACY_NUMERIC_COUNT|0021_gradebook|schema_migrations|frozen|immutable|expected.*migration|migration.*basename' backend/tests backend/db/migrations/README.md backend/db/migrate.py
echoecho"## structural verifier: expected legacy set pinning"
python3 - <<'PY'import astfrom pathlib import Pathfiles = [ Path("backend/tests/test_migration_naming.py"), Path("backend/tests/test_migrations.py"), Path("backend/tests/integration/test_migrations_ledger.py"),]needed = {"0021_gradebook.sql", "0021_gradebook_curve.sql"}for name, path in enumerate(files, 1): tree = ast.parse(path.read_text()) found = set() for node in ast.walk(tree): if isinstance(node, ast.Constant): if isinstance(node.value, str): rel = node.value if rel.endswith(".sql") and rel.startswith("00"): found.add(rel) elif isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "set": for arg in node.args: if isinstance(arg, ast.Set): vals = {elt.s for elt in arg.elts if isinstance(elt, ast.Constant) and isinstance(elt.value, str)} if vals - found: found |= vals print(f"{path}: has exact legacy needed files? {needed <= found}; pinned count? {bool(replacing=None for _ in [] )}")pyRepository: SaplingLearn/Sapling
Length of output: 18799
Pin the exact legacy migration basename set.
test_no_sequential_migration_has_been_added_since_the_cutover only checks len(legacy), so a rename that keeps the same file count would pass. The integration ledger test checks disk files against schema_migrations, not a frozen expected list, so an immutable legacy basename set should be asserted in the naming test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_migration_naming.py` around lines 100 - 106, Update
test_no_sequential_migration_has_been_added_since_the_cutover to assert that the
discovered legacy migration basenames exactly match a frozen expected set, not
only that their count equals _LEGACY_NUMERIC_COUNT. Preserve the existing
failure message context and ensure renames, additions, or removals of legacy
NNNN_ files fail the test.
| - All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL. | ||
| - Schema changes are append-only numbered migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. | ||
| - Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix** — `date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the stale sequential naming instruction.
Line 55 still says to add a “new numbered file.” This conflicts with the timestamp convention on Line 83. Update Line 55 to require a UTC timestamp-prefixed migration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 83, Update the migration creation guidance near the
existing “new numbered file” instruction to require a UTC timestamp-prefixed
filename using the established `date -u +%Y%m%d%H%M%S` convention. Keep the
guidance that existing sequential migrations are frozen and must not be renamed.
Uh oh!
There was an error while loading. Please reload this page.
AndresL230 added a commit
that referenced
this pull request
Aug 1, 2026
…I sweep Self-review of this PR found four things, all of which undercut the PR's own premise that the next person shouldn't have to re-derive any of this. pooler_url.py double-encoded the password. urlparse() returns it STILL percent-encoded, and quoting again turned `p%40ss` into `p%2540ss`, so the URI authenticated as the literal escape text. It fails as "password authentication failed" — indistinguishable from simply holding the wrong secret, which is the expensive kind of wrong. Supabase generates passwords with reserved characters, so this was not hypothetical; it was waiting for the next rotation. Decode then re-encode, and pin it with tests, because nothing about the output looks wrong until you try to connect. The workflow's skip-notice hardcoded `aws-0-<region>` while pooler_url.py, added in the same PR, calls that a guess and records that staging is on `aws-1-`. Verified against both live projects: staging answers only on aws-1-us-west-2, production only on aws-0-us-west-2, same region. An operator copying the notice got "Tenant or user not found" — the exact failure class this PR exists to delete. The notice now points at the dashboard and at the builder script. db/migrate.py still told operators the opposite of the PR. Its docstring said "the direct connection string, NOT the pooler" and main()'s unset-variable error routed the reader to Connection string -> Direct. This PR's own repro was running `python -m db.migrate`, so that was the one path left misdocumented. The docstring now explains why the old warning existed (it is still right about transaction mode / 6543) and why it no longer decides the answer (the direct host went IPv6-only). Same correction in CLAUDE.md, README.md, and docs/staging/setup-checklist.md — the checklist being the document someone actually follows to set staging up. Two smaller things while in the same file: the drift report's docstring said "Three sections" after a fourth was added, and main() returned 0 even while printing orphans or data blockers. That second one is a trap for the obvious next refactor — having the workflow call this script instead of duplicating its preflight — which would have silently downgraded a fail-on-orphan gate into a report nobody checks. It now exits 1 on orphans, a non-idempotent collision, or a data blocker; PENDING alone stays clean, since being behind is not drift. Also folds in the one finding from #509's review that cleared review but landed after merge: CLAUDE.md's Commands section still said "add a new numbered file", which #509's own CI guard now rejects. Verification: 1550 passed, 38 skipped (8 new). ruff clean. Drift report re-run against live staging returns exit 1 and correctly names the 3 orphans. No request-path or schema change, so the e2e lanes have nothing to exercise here; the ledger reconciliation that follows will take the full cycle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndresL230 added a commit
that referenced
this pull request
Aug 1, 2026
…t URI (#508) * ci: the migrate secret must be the session-mode pooler, not the direct URI Found while running the migration by hand: `db.<ref>.supabase.co` publishes ONLY an AAAA record. This machine has no global IPv6 address (link-local only, despite an RA default route), so a direct connection dies with "Network is unreachable" before it authenticates. Canopy already recorded this from the #481 work — "direct psycopg to staging is blocked, IPv6-only endpoint" — but the workflow I merged in #506 told you to use the direct string, which walks straight into it. GitHub-hosted runners have no outbound IPv6 either, so its first real run would have failed the same way. The pooler hosts do publish A records, so the fix is the SESSION-mode pooler (port 5432), not transaction mode (6543) which drops the session-level behaviour psycopg and DDL rely on. db/migrate.py's "NOT the pooler" warning is about transaction mode and predates the IPv6-only endpoint; session mode behaves like a direct connection. The pooler also changes the username to `postgres.<ref>`, which is easy to miss. Corrects both the header rationale and the skip notice, so the thing you read when the secret is missing points at a host that is actually reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ops: pooler-URI builder and a read-only migration drift report Both came out of actually trying to migrate staging, and both encode something that cost real time to rediscover. pooler_url.py builds the SESSION-mode pooler URI from the password already in an env file, so the secret never has to be copied by hand. It takes the pooler host PREFIX rather than a bare region, because Supabase assigns projects to numbered clusters (aws-0-, aws-1-) and the number is not derivable from the region — staging is aws-1-us-west-2, which an aws-0- assumption gets wrong. migration_drift_report.py answers the question you must answer before applying a backlog to an environment that has been touched outside the repo (#317): is the ledger merely BEHIND, or is it LYING? It reports pending files, orphans (recorded here but absent from the repo — flagging filename NUMBER COLLISIONS, the dangerous shape), and any object a pending migration would create that already exists, noting whether that migration is IF NOT EXISTS-safe or would fail the whole run. Object lists are parsed from the migration SQL itself, so there is nothing to keep in sync by hand. Read-only: runs no DDL, safe against production. Verified against a real database both ways — clean (0 pending, 0 orphans, "behind, not lying") and with staging's shape simulated (unrecorded migrations plus a colliding orphan), where it correctly names the collision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ops: drift report also checks pending UNIQUE indexes against live data A UNIQUE index is the one thing IF NOT EXISTS cannot make safe: it still fails if the rows already present violate it. That is a DATA problem, invisible to a schema diff, and it is what turns a clean-looking backlog into a half-applied run partway through. The report now parses pending migrations for CREATE UNIQUE INDEX (including the partial-index WHERE clause) and runs the equivalent GROUP BY ... HAVING count>1 against the live table, naming the offending rows. Generic — it follows whatever happens to be pending rather than hardcoding today's case. Verified both directions against a real database: clean data reports none, and an injected duplicate is caught with the row identified (0036_offering_null_section_unique -> ('rich-course-math210','summer-2026',2)). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ops): stop double-encoding pooler passwords; finish the direct-URI sweep Self-review of this PR found four things, all of which undercut the PR's own premise that the next person shouldn't have to re-derive any of this. pooler_url.py double-encoded the password. urlparse() returns it STILL percent-encoded, and quoting again turned `p%40ss` into `p%2540ss`, so the URI authenticated as the literal escape text. It fails as "password authentication failed" — indistinguishable from simply holding the wrong secret, which is the expensive kind of wrong. Supabase generates passwords with reserved characters, so this was not hypothetical; it was waiting for the next rotation. Decode then re-encode, and pin it with tests, because nothing about the output looks wrong until you try to connect. The workflow's skip-notice hardcoded `aws-0-<region>` while pooler_url.py, added in the same PR, calls that a guess and records that staging is on `aws-1-`. Verified against both live projects: staging answers only on aws-1-us-west-2, production only on aws-0-us-west-2, same region. An operator copying the notice got "Tenant or user not found" — the exact failure class this PR exists to delete. The notice now points at the dashboard and at the builder script. db/migrate.py still told operators the opposite of the PR. Its docstring said "the direct connection string, NOT the pooler" and main()'s unset-variable error routed the reader to Connection string -> Direct. This PR's own repro was running `python -m db.migrate`, so that was the one path left misdocumented. The docstring now explains why the old warning existed (it is still right about transaction mode / 6543) and why it no longer decides the answer (the direct host went IPv6-only). Same correction in CLAUDE.md, README.md, and docs/staging/setup-checklist.md — the checklist being the document someone actually follows to set staging up. Two smaller things while in the same file: the drift report's docstring said "Three sections" after a fourth was added, and main() returned 0 even while printing orphans or data blockers. That second one is a trap for the obvious next refactor — having the workflow call this script instead of duplicating its preflight — which would have silently downgraded a fail-on-orphan gate into a report nobody checks. It now exits 1 on orphans, a non-idempotent collision, or a data blocker; PENDING alone stays clean, since being behind is not drift. Also folds in the one finding from #509's review that cleared review but landed after merge: CLAUDE.md's Commands section still said "add a new numbered file", which #509's own CI guard now rejects. Verification: 1550 passed, 38 skipped (8 new). ruff clean. Drift report re-run against live staging returns exit 1 and correctly names the 3 orphans. No request-path or schema change, so the e2e lanes have nothing to exercise here; the ledger reconciliation that follows will take the full cycle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
AndresL230 added a commit
that referenced
this pull request
Aug 1, 2026
…igrations (#316, #265) (#510) * fix(db): reconcile staging's ledger by recovering three out-of-band migrations Staging's schema_migrations holds three rows whose filenames exist nowhere in this repo, and `git log --all` finds nothing for any of them — they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that, correctly, so all 11 pending migrations are stuck. That includes the fixes for #316 (avatars bucket still private, every avatar renders broken) and #265 (assignments_source_check still rejects 'gradescope', so every synced row would violate it). Neither needs new code; both need this unblocked. filename is the ledger's primary key, which leaves exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Staging then sees them as recorded-and-present, while prod and fresh local databases see them as pending and apply them for real. A timestamped name would leave the orphan in place AND re-run the DDL. All three are idempotent, because environments genuinely disagree about whether they ran. 0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql already carries the column and says so in a comment. 0032_retire_summer_2026 changes data and behaviour, and is worth reading before it reaches an environment that matters. It moves Fall 2026's start_date back to absorb the Summer window, because 0019 seeds deliberately contiguous ranges so exactly one term contains any date; deleting Summer without that leaves a 98-day hole where current_term() returns nothing and resolve_offering can't place an enrollment. Consequence: a date in the old Summer window now resolves to Fall. 0033_offering_section_not_null is the one with an actual design argument. 0036 patched NULL-section duplicates with a partial index, but NULL was never the only way to say "no section" — all three seeders write '' while resolve_offering omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in the table as ('' , NULL) and the index could not see it. Collapsing NULL into '' leaves one representation, covered directly by 0020's existing course_offerings_unique. resolve_offering needs no code change; its docstring did, since it cited 0036 by number for a guarantee that index no longer provides. 0036 stays (already applied elsewhere; applied migrations are immutable) and becomes a no-op, dropped by a timestamped migration so it can't be mistaken for the thing holding the invariant up. _LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard: these numbers were already spoken for by rows in a live ledger, so they are recovered history rather than newly claimed. The count stays closed at 48. The e2e lane caught the one real defect here: the rich seed pins an offering to summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted to fall-2026 — the same thing the migration does to real rows. The su26 ids are deliberately NOT renamed: on an existing local database the old row survives and would collide with the new one on course_offerings_unique. Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations): up/reset/playwright/oracles all 0, 33 journeys green including semester-scope, oracles 0 findings. Stops short of prod deliberately — 0032 is a product decision, and prod's ledger state needs checking first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(db): refuse to collapse sections or retire Summer when rows would collide Self-review caught two real defects in this PR's own migrations, both invisible to every lane that verified it. course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That is exactly the mixed state 0033's comment describes, since the seeders write '' and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = '' WHERE section IS NULL` collapses that pair onto one key and raises a duplicate key error. Two NULL rows for the same course+term collide the same way, and 0036 cannot prevent it because 0036 applies after 0033. 0032's summer->fall repoint has the identical shape: a course with an offering in both terms lands on an occupied key. Not exotic — resolve_offering(create=True) never sets section, so every app-created offering shares the same default. Either failure is worse than one bad statement. apply_migration runs the whole file plus its ledger INSERT in ONE transaction with no per-file recovery, so a collision rolls the migration back AND stops everything queued behind it. Neither verification lane could see this, which is the part worth remembering: the e2e replay starts from `supabase db reset`, so 0033 always ran against a zero-row table, and the hermetic suite mocks the DB layer entirely. "1553 passed, oracles 0 findings" was true and proved nothing here. Both migrations now detect the collision first and RAISE with the offending groups named. Deliberately not auto-merged: the colliding rows are two distinct offerings and enrollments/documents/notes hang off one id or the other, so choosing a survivor is a data call, not something a migration should do quietly. Verified against a scratch Postgres 15 by running the real migration files: 0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE 0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows 0032 colliding -> aborts, names the collision, summer-2026 still present 0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved Also from the review: - CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was frozen at 45. It is 48, and the reconciliation exception is now documented as the ONLY sanctioned reason to add one — previously that rationale lived just in a test comment and a PR description. - _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates, and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count guard could not catch a rename: it would stay at 48 and pass CI while silently reopening the orphan the file exists to close. - The seed comment named the wrong constraint. The upsert conflicts on (course_id, term_id, section), so course_offerings_unique is the conflict TARGET and routes into an UPDATE; a renamed id actually fails on enrollments.offering_id's FK, which has no ON UPDATE clause. - routes/onboarding.py still described resolve_offering as creating a NULL-section offering. 1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the guards in place: from-empty replay applied all 49, 33 journeys passed, oracles 0 findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Darkest-Teddy added a commit
that referenced
this pull request
Aug 2, 2026
…tion 0045/0046 -> 20260802012500_documents_file_sha256 and 20260802012600_documents_agent_result. Not cleanup — required. #509 froze the legacy NNNN_ set and tests/test_migration_naming.py pins the count, so merging main into this branch put it at 50 against an expected 48: AssertionError: expected 48 legacy NNNN_ migrations, found 50 which is exactly the collision the convention exists to prevent. These two files had already been renumbered twice on this branch (0043/0044, then 0045/0046) as other branches claimed the numbers first. Renaming is safe HERE specifically because these migrations have never been applied outside a local dev database. The ledger keys on basename, so a rename re-runs the file — which is why the 48 legacy names are frozen. Both of these are idempotent (ADD COLUMN IF NOT EXISTS, CREATE INDEX IF NOT EXISTS), and re-running them against the local stack under their new names applied cleanly with the dedup data intact. Suite: 1586 passed, 38 skipped. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9 tasks
Darkest-Teddy added a commit
that referenced
this pull request
Aug 11, 2026
This branch was written before the NNNN_ -> YYYYMMDDHHMMSS_ cutover and carried 0043-0046. main froze the legacy set at 48 files and guards the count in tests/test_migration_naming.py, so rebasing onto main took the tree to 52 and failed that guard. The four are renamed to their authoring timestamps, which preserves the original apply order. Safe to rename here specifically because the branch is unmerged: schema_migrations keys on basename, and the auto-migrate job (#506) only runs on merge to main, so these have never been recorded in a shared ledger under the old names. Local and E2E databases need a reset. The files cross-reference each other by bare number in ~23 places, including operator-facing RAISE WARNING strings. Rather than rewrite that prose, each file gets a header note mapping it back to its old name so those references still resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Darkest-Teddy added a commit
that referenced
this pull request
Aug 12, 2026
#505) * docs(spec): design for XP, levels, achievements, and leaderboards Turns the half-built achievements feature into a full growth system: an append-only XP ledger, levels mapped to the eleven growth stages, three leaderboards, an activity dashboard, and an admin wiki in the existing admin console tab. Resolves three contradictions in the source design: STAGE_MIN vs stageFor() thresholds (STAGE_MIN wins, stored in growth_stages), the XP curve (the mock's per-user numbers imply levels getting cheaper -- the stage sheet is coherent and is adopted instead, 29,800 XP to L50), and streak freezes (no mechanic exists anywhere; cut from v1). Also records that Sapling has no friends model today, so the friends leaderboard scope and its two achievements need one built. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(spec): delete the five leftover achievements instead of retiring them Jack's call: the final catalog is exactly the design's 30, so the five existing badges with no design equivalent (documents_5, documents_25, quizzes_10, flashcards_50, post_count_50) are deleted rather than kept as retired rows. user_achievements.achievement_id cascades, so any earned rows for those five go with them -- recorded explicitly in the spec. Drops the is_retired column with them; it existed only to preserve those five, and the wiki's existing delete action already covers removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(plan): implementation plan for XP, levels, achievements, leaderboards 18 tasks, TDD throughout, each ending in a commit. Revises the spec for the work-in-progress catalog Jack asked for: achievements gain a `status` ('draft' | 'live'). The design's 30 come in live; the 10 already seeded flip to draft and become the wiki's work-in-progress list. Nothing is deleted or remapped, so no earned row is cascaded away and the prod users holding first_login / documents_5 / documents_25 keep them. Drafts are invisible to users, never trigger-evaluated, and excluded from "N of M" badge counts. Two gaps found while checking the plan against the code: - nothing ever advanced users.streak_count (initialised to 0, only read), so the hero streak tile and four streak achievements would read zero forever. Task 6b adds services/streak_service.py as its sole writer. - `school` is not a column on user_profiles after the 0024 identity split, so the school leaderboard resolves peers via academics.school_peer_user_ids, the same fail-closed path GET /api/social/students uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(db): gamification schema — XP ledger, level curve, friends * docs(spec): correct the level-50 XP total to 30,000 The stage bands sum to 30,000, not 29,800 — an arithmetic slip in the prose. The growth_stages table, which is the authoritative source for level maths and what migration 0043 seeds, was always correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(db): add the design's 30 badges as live, demote the seeded 10 to draft * feat(growth): level curve derived from growth_stages growth_stages is the single source of truth for level maths — the hero card, the leaderboard's stage label and the `level` achievement trigger all read the curve through here. Cached with lru_cache plus a clear_growth_cache() hook wired into the autouse _clear_lru_caches fixture, per #98. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(growth): make band lookup independent of query sort order _band_for_level scanned _bands() with an early break, which is only correct if bands are ascending by min_level. sort_order and min_level are independent columns (0043_gamification.sql ties neither to the other) that merely happen to agree in the current seed; a future reorder of sort_order without a matching min_level change would silently return the wrong band past the divergence. - _bands() now sorts explicitly by min_level before computing spans. - Add a regression test with sort_order/min_level disagreeing, which fails on the old code (band lookup returns the wrong stage). - Reword stages() docstring: the copy is per-row shallow (dict(r)), not deep — sufficient because growth_stages rows are flat. - Comment the level_for_xp break: only the terminal band is expected to have a non-positive per-level cost; a misconfigured non-terminal band would strand the user at that level with no error, not loop. * feat(xp): idempotent award path over the xp_events ledger Single chokepoint for every XP grant: writes to the append-only xp_events ledger keyed on a deterministic idempotency_key, then refreshes users.total_xp/level. A 409 from the unique index means already-paid and is a clean no-op that leaves users untouched. award_xp_safe wraps this for request paths that must not fail on XP bookkeeping. * feat(xp): award XP from quiz, upload, note and session completion Wires services.xp_service.award_xp_safe into the four routes that earn XP, each keyed on the id of the row just persisted (attempt/document/ note/session id) so retries are idempotent no-ops rather than double payouts. documents.py awards inside the shared _persist_document helper so the streaming /upload and /upload/sync twin each pay out exactly once per logical upload. Pre-existing check_achievements calls in quiz.py/learn.py (an older achievement system) were left as-is, no duplicate imports added. * test(xp): pin the four XP award payloads at the route level The prior test_xp_wiring.py only exercised services.xp_service directly; the 190 route tests pass regardless of a typo'd rule_key or a swapped source_id because the autouse hermetic Supabase client makes any unmocked services.xp_service.table call a silent no-op. Add one route test per earning path (quiz submit, document upload/sync, note create, session end) that patches services.xp_service.table with an enabled rule and asserts the exact xp_events.insert payload against an independently-known earning id, plus one negative test (the pending- session early return) proving a failed/unpersisted action awards nothing. Test-only; no route code changed. * feat(achievements): new trigger types and XP payout on unlock Adds the 13 new trigger types for the 30-badge catalog seeded by migration 0044 (flashcards_reviewed, concepts_mastered, courses_with_mastery, graph_nodes_count, friends_count, level, session_minutes, session_before_hour, session_after_midnight, xp_in_day, goal_streak, owned_room_members, rooms_active, room_replies). check_achievements now resolves and status-checks the achievement row BEFORE granting, so 'draft' badges (10 pre-existing ones left in that state by 0044) can never be awarded to a user. On a live grant it now pays the badge's xp_reward via xp_service.award_xp_safe. Return shape changes from list[str] slugs to list[dict] ({"slug", "name", "xp"}) — verified by grep that every existing call site (admin.py, auth.py, documents.py, flashcards.py, learn.py, quiz.py, social.py) discards or merely forwards the list without treating elements as strings, so no caller changes were required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(achievements): clamp goal_streak against non-positive goals; add course_grade_a trigger Review follow-up on the achievement-trigger work: - _goal_streak now clamps the stored daily_goal_xp to at least 1. The column has no CHECK enforcing positivity (0043), and int(... or 50) only guards falsy values, not negatives — a stored goal of 0 or less made `0 >= goal` trivially true forever, so the backwards day-walk never terminated (confirmed via RED: an unclamped -1 goal blows through the loop until date underflows with OverflowError). - Adds the course_grade_a trigger (migration 0044 seeds a 'grade-a' badge on it that was previously unearnable — _get_user_stat had no branch for it and silently fell through to 0). Counts the user's enrollments whose current computed letter grade is an A variant (A, A-, or A+ under a custom scale), reusing gradebook_service.current_grade / letter_for rather than reimplementing the grade math, with points_possible/points_earned decrypted the same way routes/gradebook.py's _load_assignments does. Also pins the session_before_hour "hours before 24" inversion with an explicit unit test per review request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(streaks): advance streak_count and longest_streak; hide drafts from users users.streak_count was initialised to 0 and only ever read; nothing advanced it, so streak-based achievements (on-fire, marathon, wildfire) and the hero card streak tile read zero forever. services/streak_service.py::touch_streak is now the sole writer, called via touch_streak_safe from routes/learn.py's end_session beside the session_completed XP award (same commit point, same pending-session early return, same-day idempotency mirroring the xp_events key). touch_streak is idempotent within a UTC calendar day, increments on a consecutive day, resets to 1 after a gap, and longest_streak only ratchets up. Also scopes routes/profile.py's get_achievements to status=live so migration 0044's draft badges stop appearing in user-facing reads; user_achievements rows are untouched, so a badge earned before being drafted keeps its row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(streaks): delete graph_service's duplicate streak writer Review of task 6b found that services/graph_service.py::update_streak already advanced streak_count from apply_graph_update on every mastery change (reached from learn, notes, documents, quiz and agent tool code) — using local date.today() and never writing longest_streak. Racing against streak_service.touch_streak's UTC calendar day on the same two columns let streak_count flap between a UTC-day and local-day value depending on which path wrote last, and left longest_streak lagging the true peak. graph_service.apply_graph_update's mastery-change branch now delegates to streak_service.touch_streak_safe instead of a duplicate implementation, so there is exactly one definition of "a study day." No import cycle: streak_service only imports db.connection, confirmed by direct import. Adds test coverage in test_graph_service.py: a dedicated assertion that a mastery change advances streak_count/longest_streak via streak_service (UTC semantics), and a reconciliation test (TestStreakReconciliation) with a stateful users-table fake proving a mastery-driven touch followed by a session-end touch on the same UTC day increments exactly once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(social): friends — requests, accept/decline, list Adds friendships/friend_requests-backed endpoints under /api/social/friends (request, accept, decline, remove, list, requests). Accept writes symmetric friendship rows and fires check_achievements for both users. Fixes a route- ordering bug present in the task brief's code block itself: GET /friends/{user_id} was written before GET /friends/requests, which would have made /friends/requests unreachable (FastAPI matches "requests" as a user_id) despite the brief's own prose asserting the opposite order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(social): close friends authorization hole with require_self guards The friends endpoints (request/accept/decline/remove/list/list-requests) trusted caller-supplied user_id with no session check, unlike every other endpoint in social.py. Concretely: anyone could accept/decline someone else's friend request, tear down another user's friendships, read any user's friends list, or send a request "as" another user by setting from_user_id. Add require_self(user_id, request) to all six endpoints, matching the file's existing convention (e.g. create_room, join_room). GET /friends/{user_id} is guarded self-only for now — a friends list may become shareable later, but the safe default until that's a deliberate product decision is owner-only. The to_user_id != user_id 403 check in _load_request stays: it answers "was this request addressed to you", which is still meaningful once identity is authenticated. Adds TestAuthGuard: one test per endpoint asserting require_self is called with the right user_id (patching routes.social.require_self on top of conftest's autouse no-op stub), plus one proving a guard rejection actually propagates as 403 rather than being swallowed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(social): reactivate stale friend_requests row instead of 500ing send_friend_request only 409d when an existing (from_user_id, to_user_id) row was 'pending'. A 'declined' row, or an 'accepted' row that survived remove_friend (it only deletes friendships rows, not the historical friend_requests row), fell through to insert() with the same pair and hit the UNIQUE(from_user_id, to_user_id) constraint from migration 0043 — db/connection.py's insert() calls raise_for_status() unconditionally, so the conflict surfaced as an unhandled 500. User-visible path: A asks B, B declines, they patch things up later, A asks again -> 500. Same for unfriend-then-refriend. Fix: when a non-pending row already exists for the pair, UPDATE it back to pending (clear responded_at, refresh created_at) instead of inserting a duplicate, returning the same {"request": {...}} shape either way. pending -> 409 is unchanged. Confirmed the accepted/declined row remove_friend leaves behind does not leak into GET /friends/requests: both incoming/outgoing selects already filter status=eq.pending explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(gamification): hero, leaderboard and activity endpoints Adds GET /api/gamification/{me,leaderboard,activity}: the read side of the XP/achievements feature, deriving hero-card progress, weekly leaderboard ranking (everyone/friends/school scopes, private-profile suppression), and 7-day/8-week activity charts from the xp_events ledger. Mounted in main.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gamification): paginate xp_events reads past PostgREST's max_rows _events_since fetched the full weekly/8-week xp_events window in a single select() with no limit. supabase/config.toml caps PostgREST at max_rows=1000, and PostgREST signals that cut with 206 Partial Content (a 2xx), so raise_for_status() never catches it -- past ~1000 platform-wide weekly events, /leaderboard totals and rank order would silently go wrong for an unpredictable subset. Page to completion via select_with_count with a deterministic created_at,id order (required for offset paging to be correct), and add a regression test that fails against a single-page read. Also covers the previously-untested scope=school leaderboard path, and leaves a comment at each make_etag() call noting daily_goal_xp isn't yet an etag input (inert today; nothing writes that column). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(admin): 512x512 achievement icon upload with header-parsed validation Adds POST /api/admin/achievements/{id}/icon: admins upload a base64 PNG/WebP/SVG, storage_service.validate_icon parses width/height from the file header (never a client-supplied field) and rejects anything off-spec, malformed, or truncated with a clean 400. Widens the update_achievement allowlist to cover icon_url/xp_reward/sort_order/status added by migration 0043. * fix(admin): anchor SVG icon validation to the root <svg> element, add WebP coverage _svg_is_square previously accepted the first viewBox= match anywhere in the first 4KB, so a square decoy in a leading comment or on a nested <symbol>/<pattern>/inner <svg> could slip a non-square root icon past server-side validation. It now skips leading whitespace/XML decl/DOCTYPE/ comments, requires the next tag to be the root <svg>, and only reads viewBox from that tag's own attributes — failing closed (reject) on anything else, same as the PNG/WebP header parsers. Also adds a real RIFF/WEBP container builder and coverage for all three _webp_dimensions variants (VP8X/VP8 /VP8L), a wrong-dimension case, and a truncated-header case, which had no tests before. Documents why the shared len(data) < 30 guard is deliberately the max across variants rather than per-variant. * fix(admin): refuse to grant a draft achievement Task 6 stopped the automatic trigger checker from granting non-live badges, but the manual POST /achievements/grant admin path still bypassed that check. Look up the achievement's status before the user_achievements insert: 404 if it doesn't exist, 409 naming the slug and telling the admin to publish it if the status isn't 'live'. Preserves the existing already-earned skip and check_achievements call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(admin): editable XP rules Add GET /xp-rules and PATCH /xp-rules/{key} so admins can retune per-action XP amounts and disable rules without a deploy; services/xp_service.py reads xp_rules at award time. PATCH validates amount >= 0 and rejects an empty body, both 400. UpdateXpRuleBody lives in the models/ package alongside the other admin body models. Note: the routes/admin.py route bodies for this landed in the previous commit (ba6552a) because `git commit <pathspec>` snapshots the working-tree content of matched paths, and admin.py had both this and the grant-gate change pending at once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(admin): assert no user_achievements write on grant-gate rejection test_404_if_achievement_missing and test_409_if_achievement_is_draft previously asserted only the HTTP status/detail, so a handler that inserted into user_achievements before 409ing would have passed both. Assert table() was never invoked with "user_achievements" in either case, following the t.return_value.update.assert_not_called() convention in test_cannot_unapprove_self. Verified RED by temporarily moving the insert above the gate in routes/admin.py, confirming both tests fail on the new assertion (not the status code), then reverting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(frontend): badge art compositor and pre-rendered growth stages Task 11: adds the visual primitives the gamification UI renders with — rarity disc palettes + BadgeArt icon compositor (icon_url > built-in > emoji > star precedence), 11 pre-rendered growth-stage medallion SVGs, and the GrowthStage/GamificationMe/LeaderboardRow/ActivityData/Friend types plus xp_reward/icon_url/sort_order on Achievement. Icon paths and stage art are transcribed verbatim from the design source via throwaway extraction scripts (deleted after use, not shipped). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(frontend): API client for gamification, friends and XP rules Thirteen functions over the endpoints built in Tasks 7-10: the three gamification reads, the six friends endpoints, and admin XP-rule editing plus achievement patch/icon-upload. Follows the file's existing fetchJSON + exported-const convention; every interpolated path segment and query value is encodeURIComponent'd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(frontend): growth hero card and category badge grid Adds the Achievements screen's growth hero card, category badge grid, unlock modal, and the achievements/leaderboard/activity tab shell (leaderboard + activity are Task 14 placeholders). Preserves the existing five-slot showcase and its drag-to-reorder logic unchanged. * fix(frontend): restore category filter pills on Achievements Review found that BadgeGrid's per-category heading + count chip is a label, not a scoping control — removing the old FilterPills row cost users with a large catalog the ability to jump straight to one category. Brings back FilterPills (the shared component, not a new control), backed by a filter state on Achievements.tsx and a new optional `category` prop on BadgeGrid that scopes it to one section. Also adds a "No achievements yet." fallback for an empty catalog or an empty filtered category, where BadgeGrid previously rendered nothing. * feat(frontend): leaderboard and activity tabs Task 14: replaces the Achievements screen's Leaderboard/Activity placeholders with real fetchLeaderboard/fetchActivity-backed views — scoped podium + ranked list with a weekly reset timer, and the weekly/8-week bar charts (barHeights helper) with goal-line scaling. * fix(frontend): extract and test podium/goal-line math, drop rank-based check Code review follow-up for Task 14: - Extract buildPodiumSpots (LeaderboardTab) and computeGoalY (ActivityTab) into pure, exported functions and unit-test them the same way barHeights already was — podium at 0/1/2/3+ rows, goal line vs. bar height agreement at 0 and non-zero scale. - Replace the viewer-membership check's rank > rows.length heuristic with an explicit user_id membership check, since rank contiguity isn't guaranteed by the LeaderboardRow type. * feat(admin): achievement wiki with icon upload and XP rules Replaces the old AchievementsTab in place. Drafts get their own Work-in-progress section with a Publish action; live badges can be unpublished. Inline editing covers name, description, category, rarity, XP reward, sort order and the secret flag, with a client-validated 512x512 icon drop zone and the XP-rules panel below the grid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(admin): stale-closure edits, cosmetic linking, and icon-upload gating on the achievement wiki Task 15 review fixes: - resetEditFields depended on [achievement.id], freezing the edit form's closure at the card's first render forever; re-expanding after a save repopulated stale pre-save values, and a later save silently reverted the earlier one. Now depends on the whole achievement object and the exhaustive-deps suppression is gone. - Restored achievement<->cosmetic link/unlink, dropped when AchievementsTab was replaced with no note that the capability was being cut. - Gated the icon drop zone's click/drop against concurrent uploads. - Exported readIcon and added direct unit tests for its validation branches. Co-Authored-By: Claude Sonnet 5 (1M context) <noreply@anthropic.com> * feat(social): friends list, requests and add-friend action Adds a Friends panel to Social.tsx (friends list with Remove behind useConfirm, Incoming requests with Accept/Decline and a count chip, Outgoing pending list) and an add-friend action to ProfileView.tsx that checks the viewer's own friends/requests to render Add friend, Request sent, or a Friends indicator, sending via sendFriendRequest and handling 409 via the existing toast. Every mutation refetches from the server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(e2e): XP earn to hero card and leaderboard journey Adds frontend/e2e/gamification.spec.ts (earning XP moves the hero card's total-XP readout, the leaderboard, and the activity tab's week total) plus an awardXp helper in support/db.ts that ports xp_service.py's ledger-insert + total_xp/level cache refresh (including the growth_stages level curve) directly against Postgres, since there's no XP-grant API endpoint. Adds growth_stages/xp_rules to both TRUNCATE_DENYLISTs (db.ts and the pytest integration conftest) — they're migration-seeded reference tables the rich seed never re-inserts, so without the denylist entry every per-test reset would empty the level curve and rules the whole gamification UI depends on. Also fixes the testid convention drift Task 16 left behind: Social.tsx's bare friend-* ids are renamed to social-friend-* (one prefix per surface, per docs/frontend-testids.md), and the profile surface (ProfileView.tsx) plus the new achievements surface (Achievements.tsx + HeroCard/ LeaderboardTab/ActivityTab) are registered in that doc and added to the eslint testid-enforcement file list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gamification): guard the XP/leaderboard/activity reads with require_self /api/gamification/me, /leaderboard and /activity took user_id as a query parameter and called no guard. The module never imported auth_guard and main.py mounts the router without dependencies=, and there is no global auth middleware — so with no cookie at all, `leaderboard?scope=everyone` enumerated every user with app-decrypted display names, levels, XP and streaks; `/me?user_id=<victim>` returned their stats; and `leaderboard?user_id=<victim>&scope=friends` returned the victim's entire friends list. require_self(user_id, request) is now the first statement of all three handlers, matching routes/social.py and routes/profile.py. profile.py::get_achievements had the same shape and no guard (pre-existing, not introduced by this branch) — guarded too. Its payload is per-user (earned state plus progress counters computed for user_id) and the frontend only ever calls it for the signed-in user. Also closes two payload bugs on that endpoint. The select omitted xp_reward, icon_url, sort_order and status, all of which the frontend Achievement type declares required; the response is cast and never validated, so tsc passed while BadgeGrid/BadgeModal rendered "+undefined XP" and iconUrl={undefined}, making every admin-uploaded icon invisible to users. And the showcase embed in _get_featured_achievements had no status filter (it needs achievements!inner before a filter can reach the embedded table), so badges the wiki unpublishes stayed pinned on the showcase while being absent from the grid and the "N of M" count. export_data stays unfiltered on purpose — a user's own data export should include everything they earned. conftest stubs require_self to a no-op for every test, so the guard tests re-patch it per-module and assert both the call and that a rejection propagates, the way test_friends_routes.py::TestAuthGuard does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(xp): recompute total_xp from the ledger instead of incrementing it users.total_xp is a cache of the append-only xp_events ledger, but award_xp derived it as prev_total + value despite the docstring claiming it refreshes from the ledger. That is a read-modify-write with no lock: two concurrent awards read the same prev_total and the second UPDATE discards the first, permanently, with nothing anywhere to reconcile it. It is user-visible, not just theoretical: /activity and the xp_in_day / goal_streak achievement triggers sum xp_events directly, while /me and the leaderboard read the cache — so a user sees an activity chart totalling more XP than their hero card claims. _ledger_total() now sums the user's whole ledger through the same select_with_count pagination pattern routes/gamification.py uses, with the same _XP_EVENTS_PAGE=1000 cap and the same created_at,id stable sort. Both matter: PostgREST signals a truncated page with 206 Partial Content, a 2xx, so raise_for_status never fires and a single unbounded select would have silently reset a heavy user's total to the first page's sum. Recomputing also makes the cache self-healing — whatever it drifted to, the next award lands it back on the ledger sum. Not using a PostgREST server-side aggregate (select=amount.sum()): it would be cheaper but depends on db-aggregates-enabled, and if that is off PostgREST 400s, award_xp raises, award_xp_safe swallows it and XP silently stops being cached. Test fixture consequence: xp_events stubs that were bare MagicMocks now need a real select_with_count, so both shared helpers model an in-memory ledger. test_xp_wiring's mattered most — a bare MagicMock would make the tuple unpack raise *after* the insert those tests assert on, silently retiring their coverage of the tail of award_xp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(social): make accepting a friend request idempotent _load_request checked to_user_id but not status, and the friendships insert had no conflict handling against its PRIMARY KEY (user_id, friend_id). Two routes in, neither needing an adversary: 1. a plain double-click or retry on an already-accepted request; 2. mutual requests — send_friend_request only checks the exact (from, to) pair, never the reverse, so A->B and B->A both go pending; B accepts A->B, then A clicks Accept on B->A. Either way: duplicate key -> unhandled 500. And because the row stayed pending, it 500d on every subsequent retry, permanently. Accept now short-circuits when the request is not pending or the users are already friends. It still resolves the request row in that case — a bare early return would leave a stale pending row surfacing as an actionable incoming request forever. The friendships write is also an upsert on the primary key rather than an insert. The status check alone cannot close the race: a real double-click fires both requests before either has updated the status, so both read pending and only a conflict-tolerant write is actually safe. send_friend_request is deliberately left alone — mutual pending requests stay legal, now that accepting one makes the other a no-op. Auto-accepting a reverse request would silently create a friendship from a click that only meant "send a request". Also wires the room-side achievement dispatch this file owns (room_replies, rooms_active on post; owned_room_members on join/create) — see the following commit. owned_room_members is fired for the room's created_by, not the joiner: Grovekeeper is "build a room five people join", the owner's stat. The pre-existing happy-path accept test relied on an unstubbed friendships.select MagicMock reading as truthy; it now stubs it explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(achievements): dispatch the 12 trigger types nothing ever fired check_achievements only evaluates triggers whose trigger_type equals the event_type it is handed. Task 6 taught _get_user_stat thirteen new trigger types, but nothing in the product ever fired them — so 18 of the 30 badges 0044 makes live could never be earned. Worse than merely unearnable: profile.py's progress bars call _get_user_stat read-only, so a user watched a bar fill to 100/100 next to a badge that stayed locked forever. New call sites, all post-commit side effects that cannot fail the action that earned them: graph_service.apply_graph_update concepts_mastered, graph_nodes_count, courses_with_mastery flashcards.rate_card flashcards_reviewed learn.end_session session_minutes, session_before_hour, session_after_midnight, goal_streak gradebook create/update assignment course_grade_a social (previous commit) room_replies, rooms_active, owned_room_members xp_service.award_xp (earlier commit) level (on level-up), xp_in_day The graph dispatch sits at the end of apply_graph_update rather than beside touch_streak_safe inside `if mastery_changes:` — graph_nodes_count grows when nodes are created, which happens on updates that change no mastery at all. course_grade_a did have a clean hook after all: a letter grade is derived and never stored, so there is no "grade recorded" row, but every write that can move the computed percent goes through create_assignment or update_assignment_route. No hook was invented. Recursion: check_achievements pays xp_reward through award_xp_safe, which re-enters award_xp, so a level-up badge that pays enough XP to level you up again would recurse without bound. award_xp's dispatch sits behind a threading.local re-entrancy flag cleared in a finally — thread-local because the app is threaded and one request's dispatch must not suppress another's, and the finally because a dispatch that raises must still release the flag or every later award on that thread silently stops dispatching. Depth is always 1. The cost is that a payout which itself causes a level-up is not noticed until the user's next award: bounded and self-correcting, versus a loop. check_achievements now drops already-earned triggers before evaluating the stat. Several of these stats are expensive (course_grade_a walks every enrollment's assignments; xp_in_day and goal_streak scan the whole xp_events ledger) and they now run on request paths — flashcard ratings, room posts, grade writes. Once a badge is earned its stat cannot change the outcome. The tests assert the CALL SITE EXISTS — they drive the real route/service and assert check_achievements was called with the expected event type — rather than that _get_user_stat returns the right number, which is the gap that let this survive 18 reviews. test_every_live_trigger_type_has_a_dispatch_site is the durable backstop: it diffs 0044's trigger types against every check_achievements literal in routes/ and services/, so a new trigger type added without a call site now fails the suite. test_the_migration_parses guards the guard, since a regex matching nothing would make it vacuous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(admin): pay the badge's xp_reward on a manual grant grant_achievement inserted the user_achievements row directly and awarded nothing, while check_achievements pays xp_reward through award_xp_safe on the earned path. mentor, comeback, secret and methuselah are manual-grant-only, so they paid 0 instead of 980 XP and two users with identical badge sets ended up with different totals. Uses the same rule_key and source_id as the earned path, so the shared xp_events idempotency key makes a re-grant (or an earned-then-granted badge) a clean no-op rather than a double payout. award_xp_safe, so XP cannot fail the grant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(storage): bootstrap the bucket with the icon MIME types too The lifespan created the bucket with allowed_mime_types=ALLOWED_CONTENT_TYPES (jpeg/png/webp/gif), but ICON_CONTENT_TYPES also carries image/svg+xml. Supabase enforces the bucket's MIME list even for service-role writes, so validate_icon accepted an SVG and the PUT then 400d — the admin saw "502 Icon upload failed (Supabase 400)". That contradicts the recorded product decision that SVG stays supported. The bootstrap list is now the union. OPERATOR ACTION, not fixable in code: ensure_bucket_exists treats a 409 as success and deliberately does not overwrite settings, so this only takes effect on buckets this code creates. Staging and prod buckets predate the change and need a one-off bucket update to add image/svg+xml before SVG icon uploads work there. Recorded as an OPERATOR NOTE in main.py's lifespan block so it is discoverable from the code. The existing lifespan test asserted the MIME list equalled ALLOWED_CONTENT_TYPES — that assertion is what pinned the bug in place, so it now asserts the union and that image/svg+xml is present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(db): 0045 surfaces 0044's over-broad trigger DELETE 0044 rebuilds triggers with a DELETE scoped to status = 'live', on the assumption that the live catalog is exactly the ten 0007 seeds it demotes one statement earlier — it is not scoped to those slugs. POST /api/admin/achievements has shipped for a while and creates rows defaulting to status = 'live', so any achievement an admin created through the wiki lost its triggers and became a live, visible, permanently unearnable badge. 0044 is already applied, so it is not edited. 0045 is deliberately DETECTION AND DOCUMENTATION ONLY and mutates nothing: - A data-only repair is not expressible. The triggers were DELETEd; achievement_triggers has no history, tombstone or audit trail, so the (trigger_type, threshold) pairs an admin configured are gone. There is nothing to reconstruct them from, and guessing in SQL is worse. - Auto-demoting affected rows to 'draft' WAS expressible and was rejected: grant_achievement 409s on a draft, so it would silently break an admin who created a badge specifically to hand out manually. So it RAISEs a WARNING naming any live achievement with zero triggers, and is a no-op on any catalog that only ever held the seeded slugs — i.e. every environment where no admin used the wiki before 0044 ran. The header documents the hazard, why it is unrepairable, why demotion was rejected, the operator pre-flight (SELECT slug, status FROM achievements) and the manual wiki repair. No user_achievements rows were touched, by 0044 or by 0045 — nobody lost a badge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(achievements): page the xp_events scan behind xp_in_day/goal_streak _daily_totals did an unbounded select("amount,created_at") over the whole ledger. PostgREST caps a response at max_rows = 1000 (supabase/config.toml) and signals it with 206 Partial Content, which is a 2xx, so db/connection.py's raise_for_status never fires and the truncation is silent. That was latent until the last wave made award_xp dispatch xp_in_day on every award. Past ~1000 lifetime events, golden-hour (xp_in_day >= 500) was computed over an arbitrary truncated prefix and became unearnable, and perfect-week (goal_streak >= 7) could spuriously reset. Pages via select_with_count with a deterministic created_at.asc,id.asc order, matching xp_service._ledger_total (same call path) rather than inventing a third idiom. Deliberately unwindowed: _best_day_xp is an all-time max, and a window would cap goal_streak at the window length, breaking any admin-configured threshold above it. Cost is bounded by _ledger_total, which already pages the same ledger on every award. Test: TestDailyTotalsPaging stubs a full page followed by a short page and asserts both are accumulated, so it fails if only the first page is taken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(social): dispatch achievements from the public-room join path POST /api/social/public-rooms/{room_id}/join fired no check_achievements at all, while the invite-code join_room fires two. Effect: room-leader (Grovekeeper, "create a study room five people join") never advanced for an owner whose members arrived through the invite-less #405 public join, so the badge was earnable or not depending on which join button the fifth member pressed. Mirrors join_room exactly: rooms_joined for the joiner, owned_room_members for rooms.created_by (the owner, NOT the joining user). The handler's select only returned id,is_public, so created_by is now selected too. Dispatch is fired unconditionally rather than only on a fresh membership, matching join_room, and wrapped so a broken dispatch cannot fail the join. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(db): 0046 recovers the triggers 0045 wrongly called unrecoverable 0045 states the triggers 0044 deleted "are simply gone - there is nothing in the database to reconstruct them from", and tells the operator to re-add each badge's trigger in the wiki from memory. That is false. Every admin-created trigger was written through routes/admin.py's trigger endpoints, all three of which log to admin_audit_log (0010) with trigger.create / trigger.update / trigger.delete and a payload carrying achievement_id, trigger_type and trigger_threshold. The exact values were on disk the whole time. 0044 and 0045 are already applied and are not edited. 0046 supersedes 0045's guidance: it corrects the record, embeds the reconstruction query for an operator who wants to preview, and performs the repair. It REPAIRS rather than only documents because the values are exact rather than inferred, and the guards make it unable to do harm: it only writes to an achievement that is status='live' AND has ZERO achievement_triggers rows, so it can never overwrite, duplicate or contradict a configured trigger or touch a draft. Rows are restored under their ORIGINAL trigger id with ON CONFLICT (id) DO NOTHING, so it is idempotent twice over and admin_audit_log.target_id keeps resolving. A trigger an admin deleted through the wiki has a trigger.delete tombstone and is deliberately NOT resurrected. Documentation-only was rejected because 0045 already tried that and the advice never reached anyone. Which is the second half of the finding: 0045's only output is a server-side RAISE WARNING and db/migrate.py registered no psycopg notice handler, so the message was discarded before an operator could read it. run() now attaches print_notice, routing NOTICE to stdout and WARNING/ERROR to stderr. Only 0027 and 0045 RAISE anything today and both are applied everywhere, so no existing environment's output changes - it just stops discarding future ones. Verified against the real local schema in a rolled-back transaction: a wiped trigger is restored with its latest PATCHed threshold under its original id, a deliberately-deleted one is not resurrected, an achievement that already has a trigger is untouched, and a second run is a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(achievements): count junction-table rows by user_id, not a nonexistent id _count_rows selected "id". room_members is PRIMARY KEY (room_id, user_id) (0001_baseline_schema.sql) and friendships is PRIMARY KEY (user_id, friend_id) (0043_gamification.sql) — neither table has an id column. PostgREST answers a projection over a missing column with 400 (42703), which db/connection.py raises, so rooms_joined, owned_room_members and friends_count did not return a wrong count, they threw. study-circle, room-leader (Grovekeeper), first-friend and popular were unearnable. That also made a55f5b5 inert: the public-room join now dispatches owned_room_members, but the stat it dispatches raised before it could count. Selects user_id instead, which is present on all eight tables _count_rows is called with, including the one call that filters by room_id. Second half: accept_friend_request called check_achievements unguarded, unlike every other dispatch site in social.py (:64, :144, :185). The friendship rows and the request status are committed before it, so the 42703 surfaced as a 500 on a friend-accept that had in fact succeeded. Wrapped to match its siblings. Every existing test in test_achievement_service.py replaces `table` with a bare MagicMock, so the requested column was never checked against anything — which is how this shipped. TestCountRowsSelectsAnExistingColumn gives the fake the real column sets from the migrations and raises PostgREST's 42703 on an unknown projection. Verified RED: all three fail with "column room_members.id does not exist" against the previous line. Found by the scoped re-review of 1230653..90abe49. Pre-existing, not introduced by that range. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scripts): make the local/E2E stack scripts runnable on Windows Three Linux-only assumptions made `make e2e-up` unrunnable under Git Bash, so the E2E lane had never been executed on this machine. 1. DOCKER_HOST. Every one of the four scripts exported unix:///run/user/$(id -u)/podman/podman.sock whenever the podman BINARY was present. On Windows podman is present but runs in a VM the CLI reaches through its own default connection, and that socket path does not exist — so the export pointed the Supabase CLI at nothing and every command failed with "Cannot connect to the Docker daemon". Verified: `supabase status` fails with the export and succeeds without it. Replaced by set_docker_host_for_podman in local-common.sh, which additionally requires the socket to exist (-S). A pre-set DOCKER_HOST still always wins and is never recomputed, which .github/workflows/e2e.yml depends on: ubuntu-latest ships podman alongside Docker and pre-sets DOCKER_HOST to the real Docker socket. 2. venv layout. The preflight tested `-x backend/venv/bin/python` and the migrate/seed steps shelled out to `venv/bin/python`. A Windows venv puts the interpreter in venv/Scripts/python.exe, so e2e-up died in preflight. $VENV_PY is now resolved once in local-common.sh, accepts both layouts, and an explicit $VENV_PY still wins. 3. setsid. Both servers launched under setsid so the recorded PID leads a process group that e2e-down kills as a unit. Git Bash has no setsid. e2e-up now degrades to a plain background job with a printed note, and e2e-down gains a taskkill /T process-tree fallback — reached only if the PID survives the POSIX group/plain kills, so Linux and CI never touch it. Without that fallback, killing the `npm run start:test` wrapper would orphan next-server still holding :3000 and the next e2e-up would fail its port preflight. local-up.sh and local-db-reset.sh carried the same DOCKER_HOST line and are fixed identically; they already picked up $VENV_PY through migrate_reload_seed. scripts/explore.sh (Chapter 2) still hardcodes setsid and venv/bin/python and is NOT covered here. Verified on Windows: all five scripts pass bash -n, and sourcing local-common resolves VENV_PY to backend/venv/Scripts/python.exe, CONTAINER_CMD to podman, and leaves DOCKER_HOST unset. The Linux path is unchanged by construction: the socket exists there, so set_docker_host_for_podman exports exactly what the old line did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gamification): stop serving a stale XP total for 30s after earning it Found by running the E2E lane for the first time: gamification.spec.ts failed with the hero card still reading "0 XP total" after XP was earned and the page reloaded. The DB and the API were both correct — users.total_xp was 30 and GET /api/gamification/me returned total_xp 30 when asked. The browser simply never asked. The three gamification routes shipped with http_cache.CACHE_CONTROL, "private, max-age=30, stale-while-revalidate=60". Inside that 30s freshness window Chromium answers from its own cache without a request, so the ETag — which was already correct and did include total_xp — never got the chance to invalidate anything. The backend log shows it exactly: the reload refetched /api/profile/{id}/achievements but issued no /api/gamification/me at all, and the next one landed 37 seconds later. max-age=30 is right for data the user does not immediately cause to change. It is wrong for a live counter: sub-30s feedback is the entire point of the XP surface, and a user who just finished a quiz and opened /achievements would see the old number. conditional() and cached_json() take an optional cache_control (default unchanged, so every other #99 route keeps its 30s window), and /me, /leaderboard and /activity pass REVALIDATE_CACHE_CONTROL = "private, no-cache". no-cache does not disable caching — it requires revalidation before reuse, so an unchanged read is still a cheap 304 and only the no-ask window goes away. The 304 path takes the same directive as the 200 deliberately: a 304 refreshes the stored response's headers, so returning the default there would re-grant a fresh 30s no-ask window on the next revalidation and reintroduce the bug one request later. TestLiveCountersRevalidate asserts both halves. Verified against the live stack: the journey that caught it now passes, and the full lane is 38/38 with the oracles reporting 0 findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(e2e): let the local lane run on a machine that can't use the default ports Second half of the Windows port (f36228c), all found by actually booting the lane rather than reading the scripts. 1. db/migrate.py read migration files with the platform default codec, which is cp1252 on Windows, and died with UnicodeDecodeError 0x90 partway through the chain. This is the same one-character fix as e6311f9 on fix/staging-deploy-env-hardening, applied identically so the pending merge between the two branches stays trivial. 2. Ports. This dev box excludes TCP 54288-54788 wholesale (six WinNAT/Hyper-V reservations), which swallows the API (54321), DB (54322) and Studio (54323) ports. Binding one fails with "An attempt was made to access a socket in a way forbidden by its access permissions" while every container reports healthy — the containers are fine, the host just cannot reach them. Verified directly: 54321/54322 refuse an explicit TcpListener bind, 55421/55422 accept. $SUPABASE_DB_PORT / $SUPABASE_API_PORT now drive the DB URL and the PostgREST health poll, defaulting to the documented values so Linux and CI are unaffected. Shifting supabase/config.toml's ports to match is a local, uncommitted change — the exclusion is specific to this machine. 3. $FRONTEND_PORT is overridable and passed through to `next start`, so the lane can boot while another worktree's `next dev` holds :3000. Nothing else hardcodes the frontend's own port: build:test only bakes BACKEND_URL, and the browser reaches the API same-origin through Next. The Playwright harness follows via the E2E_FRONTEND_URL that support/stack.ts already supported. 4. build:test hardcoded NEXT_PUBLIC_SUPABASE_URL, which as a command-prefix assignment beats the environment and cannot be overridden. Now ${NEXT_PUBLIC_SUPABASE_URL:-http://127.0.0.1:54321} — same default, same POSIX-shell requirement the script already had. Not changed, worth knowing for anyone repeating this on Windows: npm scripts need a POSIX shell for those prefix assignments (export npm_config_script_shell=<git-bash>), and the Playwright re-seed fixture needs E2E_SEED_PYTHON pointed at venv/Scripts/python.exe — support/db.ts already had that override, so no change was required there. Result: the lane boots and is 38/38 green with the oracles at 0 findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(e2e): track the frontend by port owner when there is no setsid e2e-down left next-server running on a clean teardown, orphaning the port. Without setsid, $! is the `npm` wrapper, and npm exits as soon as next-server is spawned. By teardown the recorded PID is already dead, so stop_tracked's `kill -0` guard skips the whole branch — including the taskkill /T fallback added in f36228c, which only runs when the PID is still alive. next-server survives, keeps the port, and the next boot cannot bind it. Observed once: `e2e-down.sh` reported success having stopped only uvicorn, and :3001 stayed held until the tree was killed by hand. By the time the health check passes, the process listening on that port is unambiguously the one we just started, so record it instead of the wrapper. Guarded to the no-setsid path — under setsid the recorded pid leads the process group and is already correct — and to netstat being present. Deliberately does NOT sweep the port by owner at teardown: that would kill whatever holds the port, which on this machine is how another worktree's `next dev` on :3000 would get taken out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(db): rename gamification migrations to timestamp prefixes (#509) This branch was written before the NNNN_ -> YYYYMMDDHHMMSS_ cutover and carried 0043-0046. main froze the legacy set at 48 files and guards the count in tests/test_migration_naming.py, so rebasing onto main took the tree to 52 and failed that guard. The four are renamed to their authoring timestamps, which preserves the original apply order. Safe to rename here specifically because the branch is unmerged: schema_migrations keys on basename, and the auto-migrate job (#506) only runs on merge to main, so these have never been recorded in a shared ledger under the old names. Local and E2E databases need a reset. The files cross-reference each other by bare number in ~23 places, including operator-facing RAISE WARNING strings. Rather than rewrite that prose, each file gets a header note mapping it back to its old name so those references still resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(db): teach the migrate test double about notice handlers Rebase fallout, and the kind a clean merge hides. main added test_migrate_work_mem.py with a _FakeConn modelling only what its own run() touched (cursor, commit). This branch's run() also calls attach_notice_handler(conn), so the double raised AttributeError before reaching the ledger and both maintenance_work_mem tests failed for a reason unrelated to what they assert. The production code is right — run() takes a psycopg.Connection, which always has add_notice_handler. The double was incomplete, so it grows the method rather than the runner growing a getattr guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gamification): Early Bird threshold, silent dispatch failures, review findings Review pass over #505, plus the CodeRabbit and static-analysis findings. Correctness - achievement_service: `session_before_hour` encoded "earlier is better" as `24 - ended.hour` behind a `hour < 12` guard, so against early-bird's threshold of 7 every session ending 00:00-11:59 UTC scored 13-24 and cleared it — "Finish a study session before 7am" was granted for finishing at 11am, and user_achievements is append-only. It is now an explicit LOWER_IS_BETTER stat reporting the earliest finish hour, so the stored threshold stays the literal hour in the badge text and keeps meaning that when an admin retunes it from the wiki. profile._progress_for skips these: a countdown to a wall-clock hour has no progress bar, and the `min(stat, target)` clamp rendered the unearned badge as 100% complete. - social.accept_friend_request: `status != "pending" or _are_friends(...)` lumped `declined` in with `accepted`, so accepting a declined request skipped the friendships upsert, still stamped the row `accepted` and returned success — a request recorded as accepted with no friendship behind it. Only an existing friendship is idempotent now; anything else already resolved is a 409. - admin.grant_achievement: a granted badge's linked cosmetics never unlocked. check_achievements("manual_admin_grant") cannot do it — the stat is a hard-coded 0 and every manual_admin_grant trigger's threshold is 1, so the skip fires on all of them — which left mentor, comeback, secret and methuselah, all manual-grant-only, unable to unlock theirs at all. Extracted achievement_service.grant_linked_cosmetics and shared it with the earned path. - gamification.leaderboard: the ETag keyed on (row count, total XP), which any reshuffle preserves (u1:100/u2:200 -> u1:150/u2:150), so first and second place could swap while every viewer kept getting a 304 for the stale order. Keyed on the ranked (id, xp) pairs instead. Observability - The five `except Exception: pass` around achievement dispatch now log via logger.exception, matching xp_service/graph_service. They must not fail the action that earned them, but swallowing silently is exactly how the `friendships.id` 400 this PR describes stayed invisible. Tests - test_storage_service: both assertions in TestBucketBootstrapCoversIcons were tautologies (`ICON <= (ALLOWED | ICON)`; `ICON - (ALLOWED | ICON)`), true no matter what main.py passes, so they would still pass if the bootstrap dropped every icon type. Replaced with one test asserting against the list the lifespan actually hands ensure_bucket_exists. Frontend - AchievementWiki: the trigger editor PATCHed on every keystroke and each reload replaced the input's value, so an out-of-order response rewrote the field mid-typing. Moved to the draft/onBlur pattern XpRulesPanel already uses, plus an in-flight guard so a double-click on Add can't create two triggers. - BadgeModal: role="dialog", aria-modal, focus moved into the panel, Tab trapped inside it, focus restored on close. - Achievements: the showcase could only be reordered by pointer drag; added move-earlier/move-later buttons. Not addressed, deliberately: CodeRabbit also asked the leaderboard to aggregate weekly XP in the database. PostgREST cannot GROUP BY without an RPC, and CLAUDE.md requires all Supabase access to go through db/connection.py::table(), so that needs a migration and a new access path — a scale follow-up rather than a merge blocker. Its one Critical finding (a "duplicate TABS declaration" in Achievements.tsx) was a false positive: TABS is declared once and tsc is clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Why
Sequential migration numbers are claimed when a branch is written but only validated when it merges, so two branches open at once routinely pick the same number.
PR #507 hit this twice in a single branch lifetime — first against
main's0042, then against an unpushed branch already holding0043/0044. The second one was invisible on GitHub entirely; it surfaced only because both branches had been applied to the same local database and the ledger listed all four side by side.New migrations now use a UTC timestamp prefix:
No shared counter, so two branches would have to be created in the same second to collide.
The existing 45 files are NOT renamed — and must never be
This is the constraint that shapes the whole change, so it's worth stating plainly.
schema_migrations.filenameis the ledger's primary key, andpending_migrationstreats any basename it hasn't recorded as unapplied. So renaming an applied migration makes the runner apply it again.That is not theoretical here:
0021_gradebook.sqlDROPs and re-CREATEs the enrollment-keyedassignmentstable. A bulk rename would destroy the gradebook on every environment that has already run it.So the two conventions coexist permanently. This PR changes what new migrations look like and adds a guard; it touches no existing migration file.
Ordering still holds, for a narrower reason than you'd guess
Sorting is character-by-character, so length decides nothing — a year-1000 timestamp would sort before a
9999_prefix, since'1' < '9'.What actually holds: every legacy file starts with
0, every timestamp this millennium starts with2, and0<2.An earlier draft of this change asserted the length-based reasoning in both the code comments and the docs. Its own boundary test falsified it. The test now pins the real reason and keeps the counter-example inline, so the next reader doesn't re-derive the wrong one. A sequential migration numbered 3000+ would break the guarantee — one more reason the legacy set is closed.
Enforcement is a test, not a note
tests/test_migration_naming.pyfails CI if a newNNNN_file appears or a prefix is unsortable. A convention documented only in prose would decay; this one can't be violated silently.The existing
test_every_migration_has_a_four_digit_numeric_prefixhad to be relaxed to accept both shapes — as written it would have rejected every timestamped migration, which is exactly the blocker that stops teams adopting this incrementally.Changes
db/migrate.pyis_valid_migration_name()— acceptsNNNN_orYYYYMMDDHHMMSS_tests/test_migration_naming.pytests/test_migrations.pydb/migrations/README.mdCLAUDE.mdNo SQL, no schema change, no migration added.
Verification
Full suite: 1542 passed, 38 skipped, 0 failures.
ruff check db/ tests/clean.Follow-ups this enables
0045/0046. Converting them to timestamps is the real first adoption and permanently ends their collision problem, but it can't happen until this merges or they'd fail the current prefix test.feat/gamification-xp-achievementsholds0043/0044and should convert too, or it will collide with the next sequential migration anyone writes.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests