Repository navigation
fix: never seed the migration baseline into a fresh database - #686
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe migration applier now detects database schema state before baseline seeding. Fresh databases execute all migrations, preexisting databases can use the baseline, and ambiguous states stop unless explicitly overridden. A disposable PostgreSQL workflow validates these scenarios. ChangesMigration baseline handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant apply-migrations.sh
participant probe-applied-migrations.py
participant PostgreSQL
Operator->>apply-migrations.sh: Run migration apply or dry run
apply-migrations.sh->>probe-applied-migrations.py: Request baseline state
probe-applied-migrations.py->>PostgreSQL: Inspect schema catalog
PostgreSQL-->>probe-applied-migrations.py: Return owned-object evidence
probe-applied-migrations.py-->>apply-migrations.sh: Return database state
apply-migrations.sh->>PostgreSQL: Seed baseline or execute pending migrations
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Independent review: MERGE WITH NITSVerified against a throwaway The three-state boundary holds, including realistic partial statesThe builder's harness exercises the partial case at one migration deep. I ran it at six depths, applying the first N baseline migrations to simulate a run that died halfway:
The boundary is exact. Sixty of sixty one still refuses, and only the complete set flips to
The override cannot beat a confident probe
The live path is unreachable, verified in code and in a container
Fail loudNo The count: 84Counted three ways rather than trusting the summary line. Eighty four distinct filenames in the run's GoTrue prerequisite confirmedWith No migration file content changed, and the ledger schema and its filename key are untouched. One claim in the builder's report is wrong, and it mattersThe report says Nits, none blocking
Merge stateGitHub reports this branch as The defect in #676 is genuinely fixed and the fix fails in the safe direction at every boundary I could construct. Merge once the conflict is resolved. |
scripts/apply-migrations.sh seeded scripts/migration-baseline.conf into public.hive_schema_migrations whenever that ledger was empty. An empty ledger is also exactly the state of a brand new database, so a fresh install recorded 61 of the 84 migration files as already applied without executing one of them, then deployed against schema that was never created and reported success. That is the state the demo box cutover to a self-hosted Supabase will be in, on a host reachable only through a self-hosted runner, where a silent skip is close to unrecoverable without a teardown. An empty ledger has two opposite meanings and looks identical in both: a database that predates the ledger and ran those migrations by hand, or a new database that has run nothing. The ledger cannot tell them apart, so the schema is asked instead. probe-applied-migrations.py grows a --baseline-state mode that reports fresh (not one owned object of any baseline migration exists), preexisting (every baseline migration's owned objects are all present) or ambiguous (anything else, including a baseline gone stale against the database in front of it). Fresh skips the seed and leaves the whole chain pending; preexisting seeds exactly as before; ambiguous aborts, records nothing, applies nothing and prints what an operator needs to do. Unknown resolves to not applied, never to applied. Extensions are excluded from that decision on purpose. supabase/postgres ships pgcrypto and uuid-ossp pre-created and deploy/supabase/init/00-extensions.sql adds vector before any migration runs, so counting an extension as evidence would make a brand new database look partly migrated and abort the very install the guard protects. HIVE_MIGRATION_BASELINE=seed|ignore is belt and braces rather than the guard itself, because the failure mode of a required flag is someone forgetting it. It is honoured only where the probe is inconclusive or could not run, and rejected outright when it contradicts the schema. A run that cannot execute the probe refuses to guess. Both this script and the probe parse the baseline file, so the probe now reports how many entries it read and the script aborts if the two counts disagree. scripts/test-apply-migrations.sh proves the behaviour against a real throwaway supabase/postgres 17.6.1.136, the image the cutover uses, one scratch database per scenario. The bound is counted, not inferred from an exit code: against an empty database the ledger ends with 84 rows of source=applied, zero of source=baseline, and the run reports 84 executed, equal to the number of files in supabase/migrations. A populated ledger behaves exactly as before, the probe is never consulted, and a ledger missing one file applies exactly that file. A half applied schema with an empty ledger aborts. Related finding, out of scope here: a genuine fresh chain replay needs GoTrue to have run first. deploy/supabase/init/00-extensions.sql creates the auth schema and the anon, authenticated and service_role roles but not auth.users, auth.uid() or auth.jwt(), so on a bare self-hosted database the chain fails at the first file, 20260328_01_identity_foundation.sql, on a missing auth.users. That is a bootstrap ordering constraint, not a baseline one. Refs #676
A probe-only change did not retrigger the migrate step's paths filter even though apply-migrations.sh now hard depends on the probe script. Adds scripts/probe-applied-migrations.py alongside the existing apply-migrations.sh and migration-baseline.conf entries so an edit to the probe is not a silent no-deploy.
692b369 to
2ca929b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/test-apply-migrations.sh (1)
128-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove each scenario log file.
run_appliercallsmktempon every invocation and never removes the previous file. The suite runs the applier about ten times, so it leaves about ten files in the temp directory. Add a cleanup trap.♻️ Proposed change
log="" status=0 +trap 'rm -f "$log"' EXIT run_applier() { # run_applier <db> [args...]; combined output in $log, exit in $status local db="$1"; shift + [ -n "$log" ] && rm -f "$log" log="$(mktemp)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-apply-migrations.sh` around lines 128 - 135, Update run_applier to clean up the previous temporary log file before replacing log with a new mktemp path, and add an EXIT cleanup trap for the final log file so every scenario log is removed when the script finishes..github/workflows/migration-applier-tests.yml (1)
70-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
breakoverexit 0in the readiness loop, and dump logs when the tests fail.Line 73 uses
exit 0to leave the readiness loop. It ends the whole step, so the current behavior is correct, butbreakstates the intent and keeps the step extensible. Separately, the run step at line 88 has no failure diagnostics. A failed scenario currently leaves no Postgres log in the run summary.♻️ Proposed change
for _ in $(seq 1 60); do if docker exec migdb pg_isready -U postgres -h 127.0.0.1 >/dev/null 2>&1; then echo "postgres is accepting connections" - exit 0 + break fi sleep 2 done - echo "::error::Postgres never became ready" - docker logs migdb | tail -40 - exit 1 + if ! docker exec migdb pg_isready -U postgres -h 127.0.0.1 >/dev/null 2>&1; then + echo "::error::Postgres never became ready" + docker logs migdb | tail -40 + exit 1 + fiAdd a diagnostics step after the scenario step:
- name: Dump Postgres logs on failure if: failure() run: docker logs migdb | tail -200🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/migration-applier-tests.yml around lines 70 - 79, Update the readiness loop around the docker exec check to use break instead of exit 0, allowing the step to continue after Postgres becomes ready. Add a post-scenario workflow step named “Dump Postgres logs on failure” that runs only when failure() is true and outputs the final Postgres logs with docker logs migdb | tail -200.scripts/apply-migrations.sh (1)
256-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
HIVE_MIGRATION_BASELINEbefore the ledger check.
decide_baselinevalidates the override value, and the script callsdecide_baselineonly when the ledger is empty. On a database with a populated ledger, an invalid value such asHIVE_MIGRATION_BASELINE=maybepasses without any message. The operator then believes the override took effect. Validate the value once, near the start of the run, and keep the decision logic where it is.♻️ Proposed change
baseline_decision="" + +# Validated on every run, not only on the empty-ledger path, so a typo is +# reported even when the ledger makes the override irrelevant. +case "${HIVE_MIGRATION_BASELINE:-}" in + ""|seed|ignore) ;; + *) echo "::error::HIVE_MIGRATION_BASELINE must be seed or ignore, got: ${HIVE_MIGRATION_BASELINE}"; exit 1 ;; +esac + decide_baseline() { local override="${HIVE_MIGRATION_BASELINE:-}" - case "$override" in - ""|seed|ignore) ;; - *) echo "::error::HIVE_MIGRATION_BASELINE must be seed or ignore, got: $override"; exit 1 ;; - esacAlso applies to: 335-341
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/apply-migrations.sh` around lines 256 - 262, Validate HIVE_MIGRATION_BASELINE near the start of the script run, before the ledger check, so invalid values fail consistently even when the ledger is populated. Reuse the existing validation logic from decide_baseline without invoking the baseline decision early; keep decide_baseline in its current location for selecting the baseline when the ledger is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/probe-applied-migrations.py`:
- Around line 253-265: Align baseline counting across scripts by defining one
duplicate-handling convention in load_baseline and documenting it, then ensure
the probe’s baseline_entries output uses that convention. In
scripts/probe-applied-migrations.py lines 253-265, update load_baseline and its
count accordingly; in scripts/test-apply-migrations.sh lines 70-71, derive
baseline_count from the probe’s baseline_entries output rather than the
exact-spacing grep, preserving consistent handling of duplicates and whitespace.
In `@scripts/test-apply-migrations.sh`:
- Around line 143-144: Update the affected run_applier calls in the scenarios
around “an override that contradicts a fresh schema is rejected” and the
referenced later cases so HIVE_MIGRATION_BASELINE and the PATH shim are
explicitly set or cleared immediately before each invocation. Ensure each call
receives only its intended temporary environment and cannot inherit values left
by a previous run_applier call.
---
Nitpick comments:
In @.github/workflows/migration-applier-tests.yml:
- Around line 70-79: Update the readiness loop around the docker exec check to
use break instead of exit 0, allowing the step to continue after Postgres
becomes ready. Add a post-scenario workflow step named “Dump Postgres logs on
failure” that runs only when failure() is true and outputs the final Postgres
logs with docker logs migdb | tail -200.
In `@scripts/apply-migrations.sh`:
- Around line 256-262: Validate HIVE_MIGRATION_BASELINE near the start of the
script run, before the ledger check, so invalid values fail consistently even
when the ledger is populated. Reuse the existing validation logic from
decide_baseline without invoking the baseline decision early; keep
decide_baseline in its current location for selecting the baseline when the
ledger is empty.
In `@scripts/test-apply-migrations.sh`:
- Around line 128-135: Update run_applier to clean up the previous temporary log
file before replacing log with a new mktemp path, and add an EXIT cleanup trap
for the final log file so every scenario log is removed when the script
finishes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0063ef8f-5729-4610-bafa-b8c148838ba7
📒 Files selected for processing (7)
.github/workflows/deploy-demo-box.yml.github/workflows/migration-applier-tests.yml.wolf/buglog.jsonlscripts/apply-migrations.shscripts/migration-baseline.confscripts/probe-applied-migrations.pyscripts/test-apply-migrations.sh
Both points come from the CodeRabbit review on PR #686. The prefix assignments before run_applier were assignments in the harness shell, not in the applier's environment, because run_applier is a shell function rather than an external command. Measured, in posix mode (how bash behaves when invoked as sh) bash 4.4 and 5.0 leave the variable set after the function returns while bash 5.1 and later do not, so HIVE_MIGRATION_BASELINE or the python3 PATH shim could carry from one scenario into every later one. run_applier now takes leading VAR=value words and hands them to env(1), which cannot leak, and every call site passes them that way. No scenario outcome changes. A leak is loud rather than silent here: the fresh database scenario runs immediately after the seed override and would be rejected for contradicting the schema, so a passing run was already proof that nothing leaked. The full harness against supabase/postgres:17.6.1.136 reports the same nine scenarios green after the change. The three readers of scripts/migration-baseline.conf now share one convention: comments start at a #, surrounding whitespace is insignificant, and an applied entry appears exactly ONCE, so a raw count and a de-duplicated count are the same number. The applier and the probe each reject a duplicate by name, the probe counts the raw list rather than a set, and the harness takes its count from the applier's own --check output instead of a spacing sensitive grep, echoing that output on failure so the message naming the bad line is not swallowed. A duplicated or differently spaced entry is now one clear error instead of three disagreeing totals and a parsers disagree abort. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Fixes #676.
The defect
scripts/apply-migrations.shseededscripts/migration-baseline.confintopublic.hive_schema_migrationswhenever that ledger was empty. An empty ledger is also exactly the state of a brand new database, so on a fresh install the script marked 61 of the 84 files insupabase/migrations/as already applied without executing one of them, then continued against schema that was never created and reported success.That is precisely the state the demo box cutover to a self-hosted Supabase will be in, on a host reachable only through a self-hosted runner, where a silent 54 to 61 file skip is close to unrecoverable without a teardown and looks like a green deploy.
Why the baseline still exists
It prevents re-running migrations on a database that predates the ledger, whose history was applied by hand and recorded nowhere. That purpose is untouched. The bug was only that "the ledger is empty" cannot distinguish that database from a brand new one.
How the two are told apart
By the schema, not by the ledger.
scripts/probe-applied-migrations.pyalready probes object by object, so it grows one mode rather than a second mechanism:It reports one of three answers.
freshpreexistingambiguousUnknown resolves to not applied, never to applied. A probe that cannot run, a missing
python3, or an unparseable answer all land inambiguousand abort.Extensions are excluded from the decision on purpose.
supabase/postgresshipspgcryptoanduuid-ossppre-created anddeploy/supabase/init/00-extensions.sqladdsvectorbefore any migration runs, so counting an extension as evidence would make a brand new database look partly migrated and abort the very install this guard protects.Both the shell script and the probe parse the baseline file. The probe now also prints
baseline_entries=Nand the shell aborts if that disagrees with its own count, so the two parsers cannot drift apart silently.The operator flag is belt and braces, not the guard
HIVE_MIGRATION_BASELINE=seed|ignoreis honoured only where the probe is inconclusive or could not run, and is rejected outright when it contradicts the schema. The failure mode of a required flag is someone forgetting it, so it is never the only thing standing between a fresh database and a silent skip.Verification
Real throwaway Postgres,
supabase/postgres:17.6.1.136, the image the cutover uses. A plainpostgresorpgvectorimage lacks thesupabase_auth_adminrole, whose absence makes20260516_07fail outright. One scratch database per scenario, checked in asscripts/test-apply-migrations.shand wired to a path gated workflow.The stated bound is counted, not inferred from an exit code. Against an empty database the ledger ends with 84 rows of
source=applied, zero ofsource=baseline, and the run's own last line reports 84 executed, equal tols supabase/migrations/*.sql | wc -l.How the live database is protected
The live demo database has a populated ledger, so
decide_baselineis never reached there and the code path is byte for byte the one that runs today. Two scenarios pin that down rather than asserting it:baseline_state=never appears in the output, and no migration executes.Additionally verified out of band, not in the checked in suite because it costs a second full chain: fully migrating a database, truncating the ledger and re-running produces
baseline_state=preexisting, seeds 61 rows and then executes the 23 remaining files cleanly against a database that already has them, ending with 61 baseline rows and 23 applied rows and exit 0. So the ledger rebuild path on the live database completes rather than aborting.Per statement idempotency is untouched. No migration file's contents changed, and neither the ledger schema nor its filename key changed.
Finding, out of scope for this fix
A genuine fresh chain replay needs GoTrue to have run first.
deploy/supabase/init/00-extensions.sqlcreates theauthschema and theanon,authenticatedandservice_roleroles, but notauth.users,auth.uid()orauth.jwt(), which GoTrue owns. Confirmed empirically onsupabase/postgres:17.6.1.136: with only that init file applied, the chain fails at the very first file,20260328_01_identity_foundation.sql, withrelation "auth.users" does not exist. Thirteen migrations foreign key toauth.usersand ten callauth.uid()orauth.jwt().The cutover therefore has to start GoTrue against the new database before running the applier, or provide those objects some other way. This PR is scoped to the baseline logic and does not attempt to solve bootstrap ordering. The test harness creates the same stand in objects GoTrue would, which is also what
.github/ci/test-db-bootstrap.sqldoes for CI.Also in this PR
migration-baseline.confclaimed a probe of "all 79 files" while the repository now holds 84. Corrected to say it covered the 79 that existed at probe time, with the five newer files pending by absence.Migration applier testsworkflow is path gated at the workflow level so an unrelated pull request never pays for the Postgres image. It is not a required check; it should earn that after a track record on main.Summary by CodeRabbit
Bug Fixes
Improvements
Documentation