Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) by AndresL230 · Pull Request #510 · SaplingLearn/Sapling · GitHub
Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) by AndresL230 · Pull Request #510 · SaplingLearn/Sapling · GitHub
Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) by AndresL230 · Pull Request #510 · SaplingLearn/Sapling · GitHub
Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) by AndresL230 · Pull Request #510 · SaplingLearn/Sapling · GitHub
Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) by AndresL230 · Pull Request #510 · SaplingLearn/Sapling · GitHub
Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) by AndresL230 · Pull Request #510 · SaplingLearn/Sapling · GitHub
Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) by AndresL230 · Pull Request #510 · SaplingLearn/Sapling · GitHub
Skip to content

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265) - #510

Merged
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger
Aug 1, 2026
Merged

fix(db): reconcile staging's ledger by recovering three out-of-band migrations (#316, #265)#510
AndresL230 merged 2 commits into
mainfrom
db/reconcile-staging-ledger

Conversation

@AndresL230

Copy link
Copy Markdown
Collaborator

Staging's migration ledger has three rows recorded under filenames that exist nowhere in this repo — and git log --all finds nothing for any of them, so they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that drift, correctly, which means all 11 pending migrations are stuck, including the fixes for #316 and #265.

on disk 45 | recorded 37 | pending 11
ORPHANS (recorded here, absent from the repo)
0019_newsletter_approved_at.sql <-- NUMBER COLLIDES WITH 0019_conventions_terms_schools.sql
0032_retire_summer_2026.sql <-- NUMBER COLLIDES WITH 0032_rooms_missing_columns.sql
0033_offering_section_not_null.sql <-- NUMBER COLLIDES WITH 0033_realtime_publish_room_messages.sql

Two of them were applied within hours of this being written (2026-08-01 01:55 and 03:51), so this is ongoing drift, not archaeology.

Why transcription, and why these exact filenames

schema_migrations.filename is the primary key, and the runner treats any basename it hasn't recorded as pending. That gives exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Then staging sees them as recorded-and-present (neither pending nor orphan), while prod and every fresh local database see them as pending and apply them for real.

A timestamped name would not work — it would leave the orphan in place and re-run the DDL.

All three are written idempotently, because environments genuinely disagree about whether they ran: staging recorded them, prod got the newsletter column via 0026_ops.sql, and a fresh database gets them here first.

What each one actually does

0019_newsletter_approved_at.sql — a no-op everywhere except the ledger. 0026_ops.sql:30 already carries the column with a comment saying "Absorbed from 0019_newsletter_approved_at (drift fix; prod already has this column)". Restored purely to clear the orphan.

0032_retire_summer_2026.sqlthis one changes data and behaviour. It deletes the Summer 2026 term and moves Fall 2026's start_date back to 2026-05-18 to close the 98-day hole that would otherwise open in 0019's deliberately contiguous date cover (current_term() resolves by date; a date in the hole resolves to no term, and resolve_offering(create=True) then can't place an enrollment). Consequence worth stating: a date like today resolved to summer-2026 before and resolves to fall-2026 after. Offerings are repointed before the term row is deleted, since course_offerings.term_id is the only FK into terms (verified against the live schema) and it's ON DELETE RESTRICT.

0033_offering_section_not_null.sqlsection becomes NOT NULL DEFAULT ''. This is the one with a real design argument behind it, below.

The section design, and why staging's version wins

0020 made section nullable and course_offerings_unique is UNIQUE (course_id, term_id, section). Plain UNIQUE treats NULLs as distinct, so two NULL-section rows for the same course+term both survive — which is what 0036 patches, with a partial unique index over WHERE section IS NULL.

But NULL was never the only way to say "no section". Every seeder in this repo writes the empty string:

writes section as
db/seed_staging.py:119""
db/seed_local_rich.py:140""
db/e2e_staging_http.py:69""
services/academics.py::resolve_offeringomits the keyNULL

So a seeded offering and a resolve_offering'd one for the same course+term were two different values, and 0036 — scoped to WHERE section IS NULL — couldn't see the pair. The duplicate-offering bug 0036 exists to prevent walks in through the '' door.

Collapsing NULL into '' removes the second door: one representation of "no section", covered directly by the constraint that was already there. resolve_offering needs no code change — it still omits the key, the DEFAULT supplies '', and a lost race still surfaces as the 409 its existing handler re-selects on. Its docstring and the 409 comment did need updating, since both cited 0036 by number for a guarantee it no longer provides.

0036 is left untouched (it's already applied elsewhere; applied migrations are immutable) and becomes a permanent no-op — a partial index over a predicate no row can satisfy. 20260801062439_drop_dead_null_section_index.sql removes it, so it can't be mistaken for the thing holding the invariant up.

The guard bump

_LEGACY_NUMERIC_COUNT goes 45 → 48, with the reasoning in the file. These aren't newly claimed numbers — the numbers were already spoken for by rows in a live ledger. The count stays closed at 48; a genuinely new NNNN_ file still fails CI.

What this unblocks

Both of these are already fixed in main and blocked solely on the backlog:

Verification

  • Backend suite: 1553 passed, 38 skipped. ruff check clean.
  • New ordering pins in test_migrations.py: 0033-before-0036 (reversed, 0036 would index live NULL rows and 0033 would then silently empty it), 0036-before-the-drop, and 0019-before-0032 (reversed, the DELETE matches nothing and 0019 re-seeds the term).
  • Full local e2e cycle — the real gate here, since it replays the whole chain against an empty database.

Note for whoever does prod

This PR deliberately stops short of prod. 0032 is a product decision about which terms exist, and it will apply there on the next run. Prod's ledger state also needs checking first — it may not have one at all, in which case db.migrate would treat all 48 as pending.

🤖 Generated with Claude Code

…igrations
Staging's schema_migrations holds three rows whose filenames exist nowhere in
this repo, and `git log --all` finds nothing for any of them — they were applied
out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of
that, correctly, so all 11 pending migrations are stuck. That includes the fixes
for #316 (avatars bucket still private, every avatar renders broken) and #265
(assignments_source_check still rejects 'gradescope', so every synced row would
violate it). Neither needs new code; both need this unblocked.
filename is the ledger's primary key, which leaves exactly one reconciliation
move that doesn't involve hand-editing a live ledger: restore the files under
their exact recorded names. Staging then sees them as recorded-and-present,
while prod and fresh local databases see them as pending and apply them for
real. A timestamped name would leave the orphan in place AND re-run the DDL.
All three are idempotent, because environments genuinely disagree about whether
they ran.
0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql
already carries the column and says so in a comment.
0032_retire_summer_2026 changes data and behaviour, and is worth reading before
it reaches an environment that matters. It moves Fall 2026's start_date back to
absorb the Summer window, because 0019 seeds deliberately contiguous ranges so
exactly one term contains any date; deleting Summer without that leaves a 98-day
hole where current_term() returns nothing and resolve_offering can't place an
enrollment. Consequence: a date in the old Summer window now resolves to Fall.
0033_offering_section_not_null is the one with an actual design argument. 0036
patched NULL-section duplicates with a partial index, but NULL was never the
only way to say "no section" — all three seeders write '' while resolve_offering
omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in
the table as ('' , NULL) and the index could not see it. Collapsing NULL into ''
leaves one representation, covered directly by 0020's existing
course_offerings_unique. resolve_offering needs no code change; its docstring
did, since it cited 0036 by number for a guarantee that index no longer
provides. 0036 stays (already applied elsewhere; applied migrations are
immutable) and becomes a no-op, dropped by a timestamped migration so it can't
be mistaken for the thing holding the invariant up.
_LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard:
these numbers were already spoken for by rows in a live ledger, so they are
recovered history rather than newly claimed. The count stays closed at 48.
The e2e lane caught the one real defect here: the rich seed pins an offering to
summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted
to fall-2026 — the same thing the migration does to real rows. The su26 ids are
deliberately NOT renamed: on an existing local database the old row survives and
would collide with the new one on course_offerings_unique.
Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a
genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations):
up/reset/playwright/oracles all 0, 33 journeys green including semester-scope,
oracles 0 findings.
Stops short of prod deliberately — 0032 is a product decision, and prod's ledger
state needs checking first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabaseBot commented Aug 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@AndresL230, you've reached your PR review limit, so we couldn't start this review.

Next review available in:21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7aaf1009-2b6f-414d-babe-894e45e4ff74

📥 Commits

Reviewing files that changed from the base of the PR and between 39204d1 and 6422966.

📒 Files selected for processing (14)
  • CLAUDE.md
  • backend/db/migrations/0019_newsletter_approved_at.sql
  • backend/db/migrations/0032_retire_summer_2026.sql
  • backend/db/migrations/0033_offering_section_not_null.sql
  • backend/db/migrations/20260801062439_drop_dead_null_section_index.sql
  • backend/db/migrations/README.md
  • backend/db/seed_local_rich.py
  • backend/db/seed_staging.py
  • backend/routes/onboarding.py
  • backend/services/academics.py
  • backend/tests/test_migration_naming.py
  • backend/tests/test_migrations.py
  • backend/tests/test_seed_staging.py
  • frontend/e2e/semester-scope.spec.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Aug 1, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend-staging6422966Commit Preview URL

Branch Preview URL
Aug 01 2026, 06:59 AM

@AndresL230

Copy link
Copy Markdown
CollaboratorAuthor

Code review

Found 4 issues:

  1. 0033's backfill can violate course_offerings_unique and abort the entire migration run. course_offerings_unique is UNIQUE (course_id, term_id, section) and Postgres treats NULL as distinct, so (c1, t1, NULL) and (c1, t1, '') coexist legally — which is exactly the mixed state this migration's own comment says exists, since the seeders write '' while resolve_offering wrote NULL. The blind UPDATE collapses that pair onto one key and raises a duplicate-key error. Two NULL rows for the same course+term collide the same way, and 0036's partial index cannot prevent it because 0036 applies after this file. db/migrate.py::apply_migration sends the whole file as one transaction with no per-file recovery, so the failure also stops every migration queued behind it. Neither verification lane can see this: the e2e replay starts from an empty table, and the hermetic suite mocks the DB.

UPDATE course_offerings SET section =''WHERE section IS NULL;

  1. 0032's repoint has the identical shape. If a course already has an offering in both summer-2026 and fall-2026 with the same section, moving the summer row onto fall-2026 collides on the same constraint. resolve_offering(create=True) never sets section, so every app-created offering shares the identical default — this is not an exotic data shape.

-- 1. Move any Summer 2026 offering into Fall 2026 before the term disappears.
UPDATE course_offerings
SET term_id ='fall-2026'
WHERE term_id ='summer-2026';

  1. CLAUDE.md and backend/db/migrations/README.md both still say the legacy NNNN_ set is frozen at 45 files. This PR makes it 48 and bumps the guard, but the rationale for the exception lives only in a test-file comment and the PR description — not in either document CLAUDE.md tells readers to trust. (CLAUDE.md says "The 45 existing NNNN_ files are frozen and must never be renamed... Full rationale in backend/db/migrations/README.md".)

Sapling/CLAUDE.md

Lines 86 to 88 in b2331aa

- All Supabase access goes through `db/connection.py::table()`. Do not instantiate `httpx` clients or import `supabase` directly elsewhere. The one sanctioned exception is `db/migrate.py`, which connects with psycopg to run DDL.
- Schema changes are append-only migrations in `backend/db/migrations/` (applied via `python -m db.migrate`); never edit an applied migration or run DDL in the Supabase dashboard. **New migrations use a UTC timestamp prefix**`date -u +%Y%m%d%H%M%S` — because sequential `NNNN_` numbers are claimed at write time and only validated at merge, so concurrent branches collide. The 45 existing `NNNN_` files are frozen and must never be renamed: the ledger keys on basename, so a rename re-runs the migration. Full rationale in `backend/db/migrations/README.md`.
- Term/offering/enrollment resolution goes through `services/academics.py`. The HTTP boundary keeps the abstract `course_id`; the graph stays on the abstract course, gradebook keys on `enrollment_id`, and study/analytics key on `offering_id`.

  1. The new seed comment names the wrong constraint. seed_offerings() upserts with on_conflict="course_id,term_id,section", so course_offerings_unique is the conflict target — it routes into an UPDATE rather than failing. A renamed id would instead fail on enrollments.offering_id's FK (no ON UPDATE, so NO ACTION) when the UPDATE tries to change a referenced id. The conclusion — don't rename the ids — still holds, but for a different reason than stated.

(OFF_CS_S26, COURSE_CS, TERM_SPRING_2026, "Dr. Ada Lovelace", "MWF 11:00", "Hall A"),
# Keeps its `su26` id: 0032 moved Summer offerings into Fall 2026, and the
# ids are opaque keys, not claims about the term. Renaming them would leave
# the old rows behind on an existing local database, where the old and new
# offering would collide on course_offerings_unique (course_id, term_id, '').
(OFF_ENG_SU26, COURSE_ENG, TERM_FALL_2026, "Prof. Maya Angelou", "MTWTh 10:00", "Hall C"),

Below the bar but worth folding in while these files are open: _PINNED_PAIRS in test_migrations.py wasn't extended for the three duplicate-prefix groups this PR creates; 0019_newsletter_approved_at.sql is pinned by no exact-name assertion, so a rename would keep len(legacy) == 48 and pass CI while silently reopening the orphan; and routes/onboarding.py:92 still describes resolve_offering as "creating a NULL-section offering".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

… collide
Self-review caught two real defects in this PR's own migrations, both invisible
to every lane that verified it.
course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres
treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That
is exactly the mixed state 0033's comment describes, since the seeders write ''
and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = ''
WHERE section IS NULL` collapses that pair onto one key and raises a duplicate
key error. Two NULL rows for the same course+term collide the same way, and 0036
cannot prevent it because 0036 applies after 0033.
0032's summer->fall repoint has the identical shape: a course with an offering
in both terms lands on an occupied key. Not exotic — resolve_offering(create=True)
never sets section, so every app-created offering shares the same default.
Either failure is worse than one bad statement. apply_migration runs the whole
file plus its ledger INSERT in ONE transaction with no per-file recovery, so a
collision rolls the migration back AND stops everything queued behind it.
Neither verification lane could see this, which is the part worth remembering:
the e2e replay starts from `supabase db reset`, so 0033 always ran against a
zero-row table, and the hermetic suite mocks the DB layer entirely. "1553
passed, oracles 0 findings" was true and proved nothing here.
Both migrations now detect the collision first and RAISE with the offending
groups named. Deliberately not auto-merged: the colliding rows are two distinct
offerings and enrollments/documents/notes hang off one id or the other, so
choosing a survivor is a data call, not something a migration should do quietly.
Verified against a scratch Postgres 15 by running the real migration files:
0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE
0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows
0032 colliding -> aborts, names the collision, summer-2026 still present
0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved
Also from the review:
- CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was
frozen at 45. It is 48, and the reconciliation exception is now documented as
the ONLY sanctioned reason to add one — previously that rationale lived just
in a test comment and a PR description.
- _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates,
and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count
guard could not catch a rename: it would stay at 48 and pass CI while silently
reopening the orphan the file exists to close.
- The seed comment named the wrong constraint. The upsert conflicts on
(course_id, term_id, section), so course_offerings_unique is the conflict
TARGET and routes into an UPDATE; a renamed id actually fails on
enrollments.offering_id's FK, which has no ON UPDATE clause.
- routes/onboarding.py still described resolve_offering as creating a
NULL-section offering.
1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the
guards in place: from-empty replay applied all 49, 33 journeys passed, oracles
0 findings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@AndresL230