Uh oh!
There was an error while loading. Please reload this page.
ci: apply pending migrations to staging on merge to main - #506
Conversation
Nothing applied them. Verified all four places it could have happened and none did: the backend image's CMD is a bare uvicorn, there is no Procfile or release step, main.py's lifespan does not migrate, and the Supabase GitHub integration reads `supabase/migrations/` — the CLI convention — which this repo does not have (only config.toml and snippets live under supabase/, and schema_paths is empty). That is why its check reports "skipping" on every PR: it is connected but has nothing it recognises. Migrations here are raw DDL under backend/db/migrations/ applied by db/migrate.py against its own ledger. So a merge shipped code whose schema had not moved, and someone had to remember to run it. #504 is live proof: it merged code that writes source='gradescope' while the CHECK still rejects that value until 0042 is applied. STAGING ONLY, deliberately. main deploys staging; prod is a separate `production` branch promotion, and auto-applying irreversible DDL to prod on merge is a different risk decision. This runner has no down migrations. Two safety properties, both exercised against the real local database rather than assumed: - No secret set -> notice + skip, so adding this file changes nothing until STAGING_SUPABASE_DB_URL exists. - Preflight refuses to apply on a drifted ledger: a missing schema_migrations table (the #317 shape) or any recorded-but-absent filename fails the job with the offending name, instead of pushing more DDL on top of a history the repo and database already disagree about. Tested three ways: healthy (45 on disk / 45 recorded / 0 pending, exit 0), injected drift (exit 1, names the ghost row), and absent ledger (exit 1). The first draft queried a `version` column; the ledger's column is `filename`, which only the real-database test caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a staging-only GitHub Actions workflow. It runs on migration changes or manual dispatch, validates migration ledger consistency, and applies migrations when the staging database secret is available. ChangesStaging migration automation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant StagingDatabase
participant MigrationLedger
participant MigrationModule
GitHubActions->>StagingDatabase: Connect with psycopg
GitHubActions->>MigrationLedger: Validate schema_migrations records
MigrationLedger-->>GitHubActions: Return ledger consistency
GitHubActions->>MigrationModule: Run python -m db.migrate
MigrationModule->>StagingDatabase: Apply repository migrations
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 | 2192652 | Commit Preview URL Branch Preview URL | Aug 01 2026, 01:42 AM |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/migrate-staging.yml (2)
46-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
set -euo pipefailto the run scripts.Neither run block sets
set -euo pipefail. In the Preflight step, ifcd backend(Line 52) were to fail for any reason, the script would continue:pathlib.Path("db/migrations").glob("*.sql")(Line 55) silently returns an empty set instead of raising, sofileswould be empty whilerecordedstill holds every ledger entry. This produces a misleading "recorded but not in repo" failure for every migration (Line 76) instead of a clear directory error, obscuring the real root cause.Add
set -euo pipefailat the top of bothrun: |blocks so an unexpected command failure surfaces immediately and clearly.🛡️ Proposed fix for fail-fast bash scripts
run: | + set -euo pipefail if [ -z "${SUPABASE_DB_URL}" ]; thenrun: | + set -euo pipefail cd backend python -m db.migrateAlso applies to: 84-86
🤖 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 @.github/workflows/migrate-staging.yml around lines 46 - 78, Add set -euo pipefail as the first command in both run blocks in migrate-staging.yml, including the block referenced near lines 84–86, so failures such as cd backend propagate immediately instead of allowing misleading migration checks to continue.
29-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden the job with least-privilege permissions and non-persisted credentials.
Two gaps in the job/checkout configuration reduce the workflow's security posture:
- No
permissions:block is set, so the job inherits the repository/organization defaultGITHUB_TOKENscope instead of the minimum required (read-only, since this job only reads code and writes DDL to an external database).actions/checkout@v4at Line 33 does not setpersist-credentials: false. The checked-out.git/configretains theGITHUB_TOKENcredential for the rest of the job, which is unnecessary exposure given this job installs a third-party pip package.zizmor flags this as an artipacked finding.
🔒 Proposed fix for least-privilege hardening
jobs: migrate: + permissions:+ contents: read runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4+ - uses: actions/checkout@v4+ with:+ persist-credentials: false🤖 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 @.github/workflows/migrate-staging.yml around lines 29 - 33, Add job-level least-privilege permissions to the migrate job, granting only read access to repository contents. Update the actions/checkout@v4 step to disable credential persistence with persist-credentials: false, while preserving the existing checkout behavior.Source: Linters/SAST tools
🤖 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 @.github/workflows/migrate-staging.yml:
- Around line 30-31: Update the migrate job to set an explicit timeout-minutes
value, and update the psycopg.connect call to include a connect_timeout value.
Keep the existing migration flow and concurrency settings unchanged while
ensuring both the overall job and database connection fail within bounded
periods.
- Around line 17-22: Restrict the workflow_dispatch path in the workflow trigger
configuration to the main branch, so manually started runs cannot target other
branches or tags. Preserve the existing push trigger and migration behavior.
---
Nitpick comments:
In @.github/workflows/migrate-staging.yml:
- Around line 46-78: Add set -euo pipefail as the first command in both run
blocks in migrate-staging.yml, including the block referenced near lines 84–86,
so failures such as cd backend propagate immediately instead of allowing
misleading migration checks to continue.
- Around line 29-33: Add job-level least-privilege permissions to the migrate
job, granting only read access to repository contents. Update the
actions/checkout@v4 step to disable credential persistence with
persist-credentials: false, while preserving the existing checkout behavior.
🪄 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: d99fa0c5-31c2-4f84-a184-4105d0ecfa05
📒 Files selected for processing (1)
.github/workflows/migrate-staging.yml
Uh oh!
There was an error while loading. Please reload this page.
| migrate: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add timeouts to bound a hung run against an unreachable staging database.
Neither the job nor the psycopg.connect() call at Line 56 sets a timeout. GitHub Actions defaults to a 360-minute job timeout when timeout-minutes is unset. Because cancel-in-progress: false (Line 27) serializes runs in the migrate-staging group, a hung connection (e.g., staging DB unreachable or blocked by a lock) can occupy the concurrency slot for up to 6 hours, delaying every subsequent migration run queued behind it.
Set an explicit timeout-minutes on the job and a connect_timeout on the psycopg connection to fail fast instead of hanging.
⏱️ Proposed fix to bound execution time
jobs:
migrate:
+ timeout-minutes: 15
runs-on: ubuntu-latest- with psycopg.connect(os.environ["SUPABASE_DB_URL"]) as c:+ with psycopg.connect(os.environ["SUPABASE_DB_URL"], connect_timeout=10) as c:Also applies to: 56-56
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 30-87: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/migrate-staging.yml around lines 30 - 31, Update the
migrate job to set an explicit timeout-minutes value, and update the
psycopg.connect call to include a connect_timeout value. Keep the existing
migration flow and concurrency settings unchanged while ensuring both the
overall job and database connection fail within bounded periods.
Code review found a real hole. workflow_dispatch lets you pick ANY branch containing the workflow file, so a migration could be applied to shared staging straight from an unmerged branch, bypassing the push-to-main gate the whole design assumes. The bypass is not the worst part. The filename lands in schema_migrations, so if the file is then edited before merging — easy, since it was only "tested" — the merge never re-applies it, and staging silently diverges from the canonical file with NO pending/orphan signal, because the recorded filename still matches. That is precisely the immutability rule CLAUDE.md states, violated without a trace. Job-level `if: github.ref == 'refs/heads/main'` closes it. Also pins psycopg to >=3.2,<4 to match backend/requirements.txt, so the runner can't silently drift onto a major the app has never run against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndresL230
commented
Aug 1, 2026
Code reviewFound 1 real issue and 1 nit, both fixed in 2192652.
The bypass isn't the dangerous part. The filename lands in Fixed with a job-level
Checked and cleared, several of which I'd flagged as suspect: the skip output gates Apply correctly (and Noted as pre-existing, not introduced here: a multi-file run has no cross-file rollback — each migration commits individually, so a mid-run failure stops the loop and is resumable. Worth knowing operationally now that it runs automatically. 🤖 Generated with Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
…t 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>
…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>
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>
#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>
Answers "shouldn't the Supabase connection handle this?" — it can't, and here's why.
Why nothing was applying migrations
Checked all four places it could have happened:
backend/Dockerfileuvicorn main:app— no release stepmain.pylifespansupabase/migrations/(CLI convention) — this repo has no such directory (supabase/holds onlyconfig.toml+snippets, andschema_paths = [])That last one is the answer to the question. The integration is connected but inert for migrations, which is exactly why its check reports "skipping" on every PR. This repo's migrations are raw DDL under
backend/db/migrations/, applied bydb/migrate.pyagainst its ownschema_migrationsledger — a deliberate choice per CLAUDE.md, not a misconfiguration.So a merge shipped code whose schema hadn't moved. #504 is live proof: it merged code writing
source='gradescope'while the CHECK still rejects that value until 0042 is applied.Scope: staging only
maindeploys staging. Prod is a separateproductionbranch promotion, and auto-applying irreversible DDL to prod on merge is a different risk decision — this runner has no down migrations. Not doing that here.Two safety properties, both exercised against a real database
Inert until you opt in. No
STAGING_SUPABASE_DB_URLsecret → notice + skip. Merging this file changes nothing until you add the secret.Refuses to apply onto a drifted ledger. This is the #317 hazard: if
schema_migrationsis missing, a naive run treats all 45 files as pending and fails recreating existing objects. Preflight fails the job instead, and also fails on any recorded-but-absent filename — the repo and database disagreeing about history is not something to pour more DDL onto.Tested three ways against the local Postgres:
Worth noting the first draft queried a
versioncolumn; the ledger's column isfilename. Only running it against a real database caught that — it would have failed on the first real merge.To enable
Add repo secret
STAGING_SUPABASE_DB_URL= staging's direct connection string (port 5432, not the pooler). Until then it no-ops.🤖 Generated with Claude Code
Summary by CodeRabbit