Uh oh!
There was an error while loading. Please reload this page.
fix(gradescope): rewire onto the enrollment-keyed schema (#265) - #504
Conversation
Records the decision Andres already made, with the reasoning corrected.
The issue's premise ("pgvector similarity can't run over ciphertext") is
wrong: match_course_chunks ranks on 1 - (embedding <=> query_embedding) and
only SELECTs chunk_text as payload, and nothing queries it by content. So
encryption doesn't block retrieval and the decision is cheaper than stated.
But the same fact makes it partial, which is the part worth recording: the
embedding can't be encrypted (pgvector computes distance over it) and is
partially invertible back to its source text. So this restores boundary
consistency with documents.extracted_text — it does not make chunks
confidential, and the ADR says so explicitly rather than letting a future
reader assume otherwise.
Decided uniform (document AND catalog chunks) so the invariant is assertable
by the existing ciphertext oracle, and because decrypt_if_present's
raw-value fallback would make a real decrypt failure indistinguishable from a
legitimately-plaintext catalog row in a mixed table.
Ids stay computed on plaintext: AES-GCM's random nonce means identical text
encrypts differently every time, so ciphertext can never be a dedup key.
Flagged that scripts/dedupe_course_chunks.py:70 re-derives ids from the
STORED text and would destroy content-addressing if run against encrypted
rows.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>routes/gradescope.py was never migrated past the DB redesign. It named FIVE things the schema doesn't have, so every link/sync/delete returned a PostgREST 400 in production — the same failure mode as #405: 1. table user_courses -> renamed to enrollments in 0020 2. gradescope_course_links.user_id / .sapling_course_id -> 0027 keys the table on enrollment_id 3. assignments.user_id / .course_id -> 0021 made assignments enrollment-scoped 4. table course_categories -> renamed gradebook_categories in 0021 5. assignments.source CHECK allowed only {manual,syllabus}, but the sync writes source='gradescope' — so even with the columns fixed there was no legal value to write. Migration 0042 widens it (strict superset, so the constraint is added VALIDATED). The issue asked to confirm the intended UX. Enrollment-keyed is right: grades land on enrollment-scoped assignments, so a link has to name the specific class instance — a retake gives two enrollments and an abstract-course link cannot say which term's assignments to write. The schema already said this; the code hadn't caught up. Per the repo convention the HTTP boundary keeps the abstract course_id and resolves inward via academics.enrollment_id_for, so the FRONTEND CONTRACT IS UNCHANGED — list_links maps enrollment rows back out to course ids. Also fixes the reason this drift survived the lane built to catch it. `RUN_INTEGRATION=1 pytest -m integration` — the documented invocation — silently ran NOTHING and exited 0. Collecting the whole tree imports tests/test_benchmark_quiz.py and tests/test_seed_quiz_fixture.py, which import scripts that call load_dotenv(".env.staging", override=True) at module import; that clobbered SUPABASE_URL for the session, so _require_local_stack skipped all 27 integration tests. The session fixture now re-asserts the local env at run time and RAISES rather than skipping when it is still wrong — the rule the file already applied to SUPABASE_DB_URL, now applied to SUPABASE_URL too. Documented form went from "1 passed, 27 skipped" to "28 passed". Verified: 4 new integration tests exercise the real column lists against real Postgres (the mocked suite structurally cannot — it agrees with whatever the caller asserts, which is why this shipped); from-empty replay for 0042; hermetic 1534 passed; browser 37/37; oracles clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | b5555d5 | Commit Preview URL Branch Preview URL | Jul 31 2026, 08:28 PM |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:16 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 (6)
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 |
…ment
Code review found a real bug in the first cut — and it was the exact retake
case I used to JUSTIFY enrollment-keying, which is why it's worth spelling out.
enrollment_id_for re-derives ONE enrollment from the current term on every
call. That's right for CREATING a link and wrong for finding one that already
exists: the seeded user holds CS101 in both fall-2025 and spring-2026, so a
link attached to the enrollment the heuristic doesn't pick was invisible to
every write path. Re-linking created a SECOND row; DELETE removed nothing
while still answering ok:true; sync 400'd "no link" for a course GET /links
was happily listing. The GET/write asymmetry was the tell — list_links
enumerates every enrollment, the writers only ever touched one.
_all_enrollments_for() now spans every enrollment in the course. upsert_link
deletes across all of them before inserting (so re-linking after a rollover
leaves exactly one), remove_link deletes across all of them, and sync_course
keys the whole run on the LINK ROW's enrollment_id rather than a fresh guess —
grades belong to the class instance the link was made against.
Two integration tests cover it, written against the enrollment the resolver
does NOT prefer so the fixture is the hostile case. Both verified red against
the pre-fix code (2 failed / 4 passed) and green after.
Also from review:
- list_links drops rows whose course can't be resolved instead of answering
sapling_course_id: null — a null there reads as a real link the UI can't act
on.
- 0042 switches to NOT VALID. The old comment justified skipping it on the
data ("nothing to grandfather"), which answered the wrong question: the
reason that matters is that a plain ADD CONSTRAINT takes ACCESS EXCLUSIVE on
assignments for the whole validation scan. NOT VALID still enforces every
new and updated row, which is all a widened set needs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>AndresL230
commented
Jul 31, 2026
Code reviewFound 3 issues, all fixed in b5555d5.
The seeded user holds CS101 in both fall-2025 and spring-2026. A link attached to the enrollment the heuristic doesn't pick was invisible to every write path: re-linking created a second row, https://github.com/SaplingLearn/Sapling/blob/2c249c2/backend/routes/gradescope.py#L406-L419 Fixed with Two integration tests cover it, written against the enrollment the resolver does not prefer so the fixture is the hostile case. Verified red against the pre-fix code (2 failed / 4 passed) and green after.
Checked and cleared: the drift audit confirms exactly five points, no sixth (the remaining Noted, not fixed: the conftest fix repairs 🤖 Generated with Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
* ci: apply pending migrations to staging on merge to main 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> * ci: restrict the migrate job to main; pin psycopg range 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Part of #265.
Five drift points, not one
routes/gradescope.pywas never migrated past the DB redesign. It named five things the schema does not have, so every link/sync/delete returned a PostgREST 400 in production — the same failure mode as #405:user_coursesenrollments(0020)gradescope_course_links.user_id/.sapling_course_idenrollment_id(0027)assignments.user_id/.course_idcourse_categoriesgradebook_categories(0021)source='gradescope'{manual, syllabus}Point 5 matters: even a perfect column fix would still have failed every insert, because the sync had no legal value to write. Migration 0042 widens the CHECK — a strict superset, so it is added VALIDATED with nothing to grandfather.
The UX question the issue asked to confirm
Answered enrollment-keyed. Grades land on enrollment-scoped
assignments, so a link has to name the specific class instance — a retake gives two enrollments and an abstract-course link cannot say which term's assignments to write. The schema already said this in 0027; the code hadn't caught up.Per the repo convention the HTTP boundary keeps the abstract
course_idand resolves inward through the existingacademics.enrollment_id_for, so the frontend contract is unchanged —list_linksmaps enrollment rows back out to course ids on the way through.The reason this survived the lane built to catch it
RUN_INTEGRATION=1 pytest -m integration— the documented invocation — ran 1 passed, 27 skipped and exited 0. It reported green having tested nothing.Collecting the whole tree imports
tests/test_benchmark_quiz.pyandtests/test_seed_quiz_fixture.py, which import scripts that callload_dotenv(".env.staging", override=True)at module import. That clobbersSUPABASE_URLto staging for the rest of the session, so_require_local_stacksaw a non-local URL and skipped all 27.pytest tests/integration/(directory form) ran fine on the same stack, which is presumably why it looked healthy.The session fixture now re-asserts the local env at run time — not only at conftest import, which happens before those script imports — and raises rather than skipping when it is still wrong. That is the rule the same file already applied to
SUPABASE_DB_URL("raises loudly rather than skipping... a silent skip would read as safe"), now applied toSUPABASE_URLtoo.Documented form: 1 passed / 27 skipped → 28 passed.
I include this here rather than splitting it because the four new integration tests are #265's regression guard, and a guard that silently skips is not one.
Verification
POST /link,GET /links,DELETE /link/{id}against real Postgres and read back with raw SQL — the assertions that would have caught the original drift. The mocked suite structurally cannot: it agrees with whatever column list the caller names.Not covered:
POST /sync/{id}drives a live Gradescope login and scrape, so its DB surface has no offline seam. Stated in the test file rather than left implicit.🤖 Generated with Claude Code