Uh oh!
There was an error while loading. Please reload this page.
ci: the migrate secret must be the session-mode pooler, not the direct URI - #508
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdded a session-mode Supabase pooler URI generator, a read-only migration drift report, and migration documentation that requires pooler connections on port 5432. ChangesStaging migration tooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 7d31c5d | Commit Preview URL Branch Preview URL | Aug 01 2026, 06:21 AM |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/migrate-staging.yml:
- Around line 15-27: Update the staging migration guidance in README.md and
docs/staging/setup-checklist.md to use the session-mode pooler URI on port 5432,
including the postgres.<ref> username, instead of the direct database URI.
Revise all related command examples and wording so operators configure the same
pooler connection required by the migration workflow.
In `@backend/scripts/migration_drift_report.py`:
- Around line 125-129: Update the conflict extraction and reporting flow around
the migration parser and the `idempotent`/`flag` logic so idempotence is
evaluated for each extracted table, column, or index operation rather than
across the entire migration SQL. Preserve the existing conflict detection, but
associate each detected conflict with its own `IF NOT EXISTS` status and render
that status in the corresponding output line.
- Line 37: Replace the direct psycopg connection in migration_drift_report.py
with the approved read-only catalog helper/API exposed through
db/connection.py::table(), preserving the script’s catalog-query behavior. If no
suitable helper exists, document and implement a narrowly scoped read-only
exception rather than reusing the migrate.py raw-DDL exception.
- Around line 49-50: Update bare() and the catalog queries in the drift-check
flow to preserve parsed schema qualification instead of filtering only by object
names. Apply the same schema-resolution rules as backend/db/migrate.py for
unqualified names, and ensure the table, column, and index checks use the
resolved schema explicitly rather than relying on the search path.
In `@backend/scripts/pooler_url.py`:
- Line 37: Update the password handling in the pooler URL construction flow to
percent-decode the value returned by urlparse(...).password before passing it to
quote with safe="". Preserve the existing URL assembly while ensuring
already-escaped characters are encoded only once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49cbc162-12c7-434d-b3a6-cb2b0fe786b4
📒 Files selected for processing (3)
.github/workflows/migrate-staging.ymlbackend/scripts/migration_drift_report.pybackend/scripts/pooler_url.py
Uh oh!
There was an error while loading. Please reload this page.
| import re | ||
| import sys | ||
| import psycopg |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
ast-grep outline backend/db/connection.py --items all
rg -n -C2 'psycopg\.connect|def table\b|schema_migrations' \
backend/db/connection.py backend/db/migrate.py backend/scripts/migration_drift_report.pyRepository: SaplingLearn/Sapling
Length of output: 4274
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== migrations script =="
cat -n backend/scripts/migration_drift_report.py
echoecho"== migrate imports/context =="
sed -n '1,110p' backend/db/migrate.py | cat -n
echoecho"== all psycopg usage =="
rg -n 'psycopg|migration_drift_report|schema_migrations|information_schema|db/connection\.py::table\b|BackendTable|SupabaseTable' backend -g '*.py'Repository: SaplingLearn/Sapling
Length of output: 16548
Add an approved read-only catalog access boundary.
backend/scripts/migration_drift_report.py opens a direct psycopg connection at line 60. The allowed backend/db/migrate.py exception is for raw DDL, not this read-only catalog-check script. Route this through a catalog helper/API that remains under db/connection.py::table() or document a narrow read-only exception before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/migration_drift_report.py` at line 37, Replace the direct
psycopg connection in migration_drift_report.py with the approved read-only
catalog helper/API exposed through db/connection.py::table(), preserving the
script’s catalog-query behavior. If no suitable helper exists, document and
implement a narrowly scoped read-only exception rather than reusing the
migrate.py raw-DDL exception.
Source: Coding guidelines
| def bare(name: str) -> str: | ||
| return name.split(".")[-1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"## locate file"
git ls-files | grep -F 'backend/scripts/migration_drift_report.py'||trueecho"## outline"
ast-grep outline backend/scripts/migration_drift_report.py ||trueecho"## relevant lines"
wc -l backend/scripts/migration_drift_report.py
sed -n '1,180p' backend/scripts/migration_drift_report.py | cat -n
echo"## references to bare/schema_migrations/public/auth"
rg -n "bare|schema_migrations|\.migrate|search_path|current_schema|public|auth|table_schema|schemaname|obj_schema" backend/scripts/migration_drift_report.py backend/db ||trueRepository: SaplingLearn/Sapling
Length of output: 19317
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'import refrom pathlib import Pathp = Path('backend/scripts/migration_drift_report.py')if not p.exists(): print('missing backend/scripts/migration_drift_report.py') raise SystemExittext = p.read_text()print('## parsed sections')for start, name in [(48, 'bare'), (61, 'report_catalog_operations'), (98, 'probe_ledger')]: lines = text.splitlines() print(f'\n--- {name} at line {start+1} ---') limit = 160 print('\n'.join(f'{i+1}: {line}' for i in range(max(0, start-limit), min(len(lines), start+limit+limit+20))))print('\n## schema/index name extraction occurrences')for m in re.finditer(r'\b[bare]\s*\(', text): print('bare call around line', text.count('\n',0,m.start('bare')+1)+1)print('bare imports:', bool(re.search(r'from.*import.*bare|import.*utils|sys.path', text)))for m in re.finditer(r'\.split\("[.]\"', text[:200]): print('split found at', text.count('\n',0,m.start())+1, 'context=', text[max(0,m.start()-80):m.end()+80])print('\n## catalog filter fields')for m in re.finditer(r'(SELECT .*?FROM .*?(\n.*?){0,8}.*?WHERE)', text, re.S): if 'catalog' in text[:m.start()] or 'migration_drift' in text[:m.start()]: passprint('index usage around ',"n/a")PYRepository: SaplingLearn/Sapling
Length of output: 354
Keep schema-qualified identifiers in the drift checks.
bare() drops schema qualification, then the catalog queries filter only by table_name, column_name, or indexname. PostgreSQL allows identically named tables or indexes in different schemas. A pending migration targeting public.foo can be reported as conflicting because auth.foo exists, and an unqualified search can read schema_migrations from an unintended search-path schema instead of the migration schema.
Use the parsed schema from qualified names. For unqualified names, apply the same schema rules used by backend/db/migrate.py and avoid relying only on the default search path. Apply this to the table, column, and index existence queries at lines 101, 109, and 118.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/migration_drift_report.py` around lines 49 - 50, Update
bare() and the catalog queries in the drift-check flow to preserve parsed schema
qualification instead of filtering only by object names. Apply the same
schema-resolution rules as backend/db/migrate.py for unqualified names, and
ensure the table, column, and index checks use the resolved schema explicitly
rather than relying on the search path.
| if hits: | ||
| found_any = True | ||
| idempotent = "if not exists" in sql.lower() | ||
| flag = "safe: uses IF NOT EXISTS" if idempotent else "!! NO IF NOT EXISTS — would fail" | ||
| print(f" {name} ({flag})") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Report idempotence for each conflicting operation.
idempotent checks whether IF NOT EXISTS occurs anywhere in the migration. A migration can contain CREATE TABLE IF NOT EXISTS and a later non-idempotent ADD COLUMN. If the column already exists, this report labels the migration safe even though db/migrate.py will fail.
Track the idempotence modifier with each extracted table, column, or index. Render the safety result for each detected conflict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/migration_drift_report.py` around lines 125 - 129, Update the
conflict extraction and reporting flow around the migration parser and the
`idempotent`/`flag` logic so idempotence is evaluated for each extracted table,
column, or index operation rather than across the entire migration SQL. Preserve
the existing conflict detection, but associate each detected conflict with its
own `IF NOT EXISTS` status and render that status in the corresponding output
line.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/scripts/migration_drift_report.py`:
- Around line 163-167: Update the duplicate-reporting block in the migration
drift report to stop printing raw values from each duplicate entry. By default,
print only the index name and duplicate count; if an explicit local diagnostic
mode is enabled, allow values through the established redaction/encryption
helpers before printing. Keep setting blocked and reporting the affected index
unchanged.
- Around line 53-56: Update RE_UNIQUE_INDEX and the migration-report parsing
flow to cover supported CREATE UNIQUE INDEX variants, including CONCURRENTLY,
USING index_type, and quoted identifiers. For any syntax the parser still cannot
recognize, classify it as unchecked instead of silently producing no data
blockers.
- Around line 155-158: Add a null-exclusion predicate to the duplicate query
built in the migration drift report: require every unique-index key column to be
non-NULL by default, but skip this filter when the parsed CREATE UNIQUE INDEX
declares NULLS NOT DISTINCT. Combine the generated predicates with the existing
where clause before the GROUP BY query in the duplicate-check flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b6054158-73ff-469b-9090-06a2e0224f23
📒 Files selected for processing (1)
backend/scripts/migration_drift_report.py
| RE_UNIQUE_INDEX = re.compile( | ||
| r"create\s+unique\s+index\s+(?:if\s+not\s+exists\s+)?([a-z0-9_]+)\s+" | ||
| r"on\s+([a-z0-9_.]+)\s*\(([^)]*)\)(?:\s*where\s+([^;]+))?", | ||
| re.I | re.S, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Inspect pending migration syntax that the current regular expression can skip.
rg -n -i --glob '*.sql' \
'create\s+unique\s+index|create\s+unique\s+index\s+concurrently|using\s+\w+|nulls\s+(not\s+)?distinct' \
backend/db/migrationsRepository: SaplingLearn/Sapling
Length of output: 2285
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== script outline =="
ast-grep outline backend/scripts/migration_drift_report.py ||trueecho"== relevant ranges =="
sed -n '1,240p' backend/scripts/migration_drift_report.py
echo"== migration samples =="forfin backend/db/migrations/0039_rag_vector_store.sql \
backend/db/migrations/0018_documents_request_id.sql \
backend/db/migrations/0020_academics_split.sql \
backend/db/migrations/0021_gradebook.sql \
backend/db/migrations/0022_unique_nulls.sql 2>/dev/null \
backend/db/migrations/0027_gradescope.sql \
backend/db/migrations/0036_offering_null_section_unique.sql;do
[ -f"$f" ] ||continueecho"--- $f ---"
sed -n '1,90p'"$f"doneRepository: SaplingLearn/Sapling
Length of output: 7436
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'import reRE_UNIQUE_INDEX = re.compile( r"create\s+unique\s+index\s+(?:if\s+not\s+exists\s+)?([a-z0-9_]+)\s+" r"on\s+([a-z0-9_.]+)\s*\(([^)]*)\)(?:\s*where\s+([^;]+))?", re.I | re.S,)cases = { "plain": 'CREATE UNIQUE INDEX IF NOT EXISTS documents_request_id_user_unique ON documents USING btree (request_id, user_id)', "concurrently": 'CREATE UNIQUE INDEX idx_unique ON users USING btree (email) CONCURRENTLY', "quoted_index_table_columns": 'CREATE UNIQUE INDEX "My Index" ON "My Schema"."My Table" ("Column One", column_two)', "nulls_not_distinct": 'CREATE UNIQUE INDEX idx_u ON users (email) NULLS NOT DISTINCT', "nulls_distinct": 'CREATE UNIQUE INDEX idx_u ON users (email NULLS DISTINCT)', "partial": 'CREATE UNIQUE INDEX idx_partial ON users (email) WHERE (active)', "with_index_params": 'CREATE UNIQUE INDEX idx_ivfflat ON courses USING ivfflat (embedding vector_cosine_ops) WITH (lists = 100)',}for label, sql in cases.items(): m = RE_UNIQUE_INDEX.search(sql) print(f"{label}: matched={bool(m)}") if m: print(" groups =", m.groups()) else: print(" sql =", sql)# Extract existing migrations that call this parser's supported formfrom pathlib import Pathfor path in sorted(Path("backend/db/migrations").glob("*.sql")): text = path.read_text(errors="ignore") for m in RE_UNIQUE_INDEX.finditer(text): print(f"EXISTS {path}:{m.start()}: idx={m.group(1)} table={m.group(2)} cols={m.group(3)[:80]} where={m.group(4)[:40] if m.group(4) else None}")PYforfin \
backend/db/migrations/0039_rag_vector_store.sql \
backend/db/migrations/0018_documents_request_id.sql \
backend/db/migrations/0020_academics_split.sql \
backend/db/migrations/0021_gradebook.sql \
backend/db/migrations/0023_graph_integrity.sql \
backend/db/migrations/0036_offering_null_section_unique.sql;do
[ -f"$f" ] ||continueecho"--- $f ---"
sed -n '1,90p'"$f"doneRepository: SaplingLearn/Sapling
Length of output: 18440
Cover the full CREATE UNIQUE INDEX grammar checked by the report.
RE_UNIQUE_INDEX only matches the CREATE UNIQUE INDEX ... ON table(columns) [WHERE ...] form. It skips USING index_type and CONCURRENTLY clauses, and it can miss quoted identifiers. Add parser coverage for these supported forms, or report them as unchecked rather than printing no data blockers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/migration_drift_report.py` around lines 53 - 56, Update
RE_UNIQUE_INDEX and the migration-report parsing flow to cover supported CREATE
UNIQUE INDEX variants, including CONCURRENTLY, USING index_type, and quoted
identifiers. For any syntax the parser still cannot recognize, classify it as
unchecked instead of silently producing no data blockers.
| dupes = conn.execute( | ||
| f"SELECT {cols_sql}, count(*) FROM {bare(table)}{where} " | ||
| f"GROUP BY {cols_sql} HAVING count(*) > 1 LIMIT 5" | ||
| ).fetchall() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"Files matching migration_drift_report.py:"
fd -a 'migration_drift_report.py'.||true
file="$(fd 'migration_drift_report.py'.| head -n 1)"if [ -n"${file:-}" ];thenechoecho"Line count:"
wc -l "$file"echoecho"Outline:"
ast-grep outline "$file"||trueechoecho"Relevant section lines 120-180:"
sed -n '120,180p'"$file"| nl -ba -v120
fiechoecho"Search for duplicate/unique report symbols:"
rg -n "dupes|count\\(\\*\\)|IS NOT NULL|NULLS NOT DISTINCT|UNIQUE""${file:-backend/scripts/migration_drift_report.py}"||trueechoecho"Search for SQL parser definitions around duplicate checks:"
rg -n "def .*duplicate|class .*Duplicate|parse|definition|constrain" backend -S ||trueRepository: SaplingLearn/Sapling
Length of output: 850
🏁 Script executed:
#!/bin/bashset -euo pipefail
file="backend/scripts/migration_drift_report.py"echo"Line count:"
wc -l "$file"echoecho"Relevant lines 1-175 with Python-line-number-style numbering:"
python3 - <<'PY'from pathlib import Pathp = Path("backend/scripts/migration_drift_report.py")for i, line in enumerate(p.read_text().splitlines(), 1): print(f"{i:4}: {line}")PYechoecho"Search relevant terms in file:"
rg -n "dupes|count\\(\\*\\)|IS NOT NULL|NULLS NOT DISTINCT|UNIQUE|unique|where|cols_sql|KEYS|RE_KEY""$file" -S ||trueechoecho"Read-only behavioral probe for PostgreSQL UNIQUE duplicate semantics from parser docs snippet if available, plus local duplicate logic shape:"
python3 - <<'PY'from pathlib import Pathimport rep = Path("backend/scripts/migration_drift_report.py")src = p.read_text()print("has null_not_distinct_term:", "NULLS NOT DISTINCT" in src)print("has null check before duplicate group:", "IS NOT NULL" in src and "GROUP BY" in src)for name, regex in [ ("RE_KEY", r"(?<=RE_KEY = re\.compile\()('|\")(.*)\\1"), ("RE_UNIQUE_INDEX", r"(?<=RE_UNIQUE_INDEX = re\.compile\()('|\")(.*)\\1"),]: m = re.search(regex, src, re.S) print(f"{name}: {m.group(2) if m else None}")PYRepository: SaplingLearn/Sapling
Length of output: 9653
🌐 Web query:
PostgreSQL unique index NULL treated as distinct nulls not distinct
💡 Result:
In PostgreSQL, the handling of NULL values in unique indexes and unique constraints is controlled by the NULLS DISTINCT or NULLS NOT DISTINCT clauses [1][2]. By default, PostgreSQL treats NULL values as distinct [1][3]. This means that a unique index or constraint will allow multiple rows to contain a NULL value in the indexed column(s), as they are not considered equal to each other [1][2]. To change this behavior so that NULL values are treated as equivalent (and thus a unique constraint will reject multiple NULLs), you can specify the NULLS NOT DISTINCT clause when creating the index or the constraint [1][4][2]. Example usage for an index: CREATE UNIQUE INDEX name ON table (column) NULLS NOT DISTINCT; Example usage for a table constraint: CREATE TABLE products ( product_no integer UNIQUE NULLS NOT DISTINCT,...); Alternatively, you can explicitly specify the default behavior by using NULLS DISTINCT [2][5]. As noted in the PostgreSQL documentation, because the SQL standard leaves null treatment in unique constraints as implementation-defined, it is important to be aware of this setting when writing portable applications [2][5].
Citations:
- 1: https://www.postgresql.org/docs/current/indexes-unique.html
- 2: https://www.postgresql.org/docs/current/ddl-constraints.html
- 3: https://www.postgresql.org/docs/18/sql-createindex.html
- 4: https://www.postgresql.org/about/featurematrix/detail/unique-nulls-not-distinct/
- 5: https://www.postgresql.org/docs/19/ddl-constraints.html
🌐 Web query:
PostgreSQL unique index WHERE NULLs not distinct where predicate
💡 Result:
In PostgreSQL, the NULLS NOT DISTINCT clause is used with unique indexes to define how NULL values are handled regarding uniqueness constraints [1][2]. By default, PostgreSQL treats NULL values as distinct, meaning a unique index (or unique constraint) allows multiple rows to contain a NULL value in the indexed column(s) [1][2]. When you specify NULLS NOT DISTINCT, the index treats NULL values as equal to one another [1][3]. Consequently, if a table has this type of unique index, it will reject any attempt to insert or update a row if another row already exists with a NULL value in that same column (or combination of columns) [1][4]. Syntax: CREATE UNIQUE INDEX index_name ON table_name (column_name) NULLS NOT DISTINCT; This clause can be applied to both manually created unique indexes and unique constraints [1][2]. It is important to note that this feature is applicable to B-tree indexes [1][4]. Additionally, you can combine this with a WHERE predicate to create a partial unique index that treats NULLs as non-distinct only for the subset of rows that satisfy the predicate [2][5].
Citations:
- 1: https://www.postgresql.org/docs/current/indexes-unique.html
- 2: https://www.postgresql.org/docs/18/sql-createindex.html
- 3: https://www.postgresql.org/about/featurematrix/detail/unique-nulls-not-distinct/
- 4: https://www.postgresql.org/docs/18/indexes-unique.html
- 5: https://www.postgresql.org/docs/19/sql-createindex.html
Exclude NULLs from unique-index duplicate checks unless the index declares NULLS NOT DISTINCT.
The report checks GROUP BY counts, so multiple rows with NULL in any key column are reported as duplicates even though a default PostgreSQL unique index allows them. Add IS NOT NULL filters for each key column unless the parsed CREATE UNIQUE INDEX includes NULLS NOT DISTINCT.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/migration_drift_report.py` around lines 155 - 158, Add a
null-exclusion predicate to the duplicate query built in the migration drift
report: require every unique-index key column to be non-NULL by default, but
skip this filter when the parsed CREATE UNIQUE INDEX declares NULLS NOT
DISTINCT. Combine the generated predicates with the existing where clause before
the GROUP BY query in the duplicate-check flow.
| if dupes: | ||
| blocked = True | ||
| print(f" {name}: {idx} WOULD FAIL — duplicate rows exist:") | ||
| for d in dupes: | ||
| print(f" {d}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not print raw duplicate key values.
d contains database values from arbitrary unique-index columns. This can place email addresses, tokens, or other sensitive values in CI logs. Print the index name and duplicate count by default. Allow raw values only through an explicit local diagnostic mode with redaction.
As per coding guidelines, “Protect sensitive columns with the encryption helpers … at read boundaries.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/migration_drift_report.py` around lines 163 - 167, Update the
duplicate-reporting block in the migration drift report to stop printing raw
values from each duplicate entry. By default, print only the index name and
duplicate count; if an explicit local diagnostic mode is enabled, allow values
through the established redaction/encryption helpers before printing. Keep
setting blocked and reporting the affected index unchanged.
Source: Coding guidelines
AndresL230
commented
Aug 1, 2026
Code reviewFound 4 issues:
Sapling/backend/scripts/pooler_url.py Lines 60 to 64 in 28103e2
Sapling/.github/workflows/migrate-staging.yml Lines 71 to 73 in 28103e2
Lines 4 to 7 in 28103e2 Lines 86 to 91 in 28103e2
Sapling/docs/staging/setup-checklist.md Lines 18 to 24 in 28103e2 Lines 235 to 237 in 28103e2 Below the reporting bar but worth noting while the file is open: the module docstring says "Three sections" while the script now prints four ( 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…t URI Found while running the migration by hand: `db.<ref>.supabase.co` publishes ONLY an AAAA record. This machine has no global IPv6 address (link-local only, despite an RA default route), so a direct connection dies with "Network is unreachable" before it authenticates. Canopy already recorded this from the #481 work — "direct psycopg to staging is blocked, IPv6-only endpoint" — but the workflow I merged in #506 told you to use the direct string, which walks straight into it. GitHub-hosted runners have no outbound IPv6 either, so its first real run would have failed the same way. The pooler hosts do publish A records, so the fix is the SESSION-mode pooler (port 5432), not transaction mode (6543) which drops the session-level behaviour psycopg and DDL rely on. db/migrate.py's "NOT the pooler" warning is about transaction mode and predates the IPv6-only endpoint; session mode behaves like a direct connection. The pooler also changes the username to `postgres.<ref>`, which is easy to miss. Corrects both the header rationale and the skip notice, so the thing you read when the secret is missing points at a host that is actually reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both came out of actually trying to migrate staging, and both encode something that cost real time to rediscover. pooler_url.py builds the SESSION-mode pooler URI from the password already in an env file, so the secret never has to be copied by hand. It takes the pooler host PREFIX rather than a bare region, because Supabase assigns projects to numbered clusters (aws-0-, aws-1-) and the number is not derivable from the region — staging is aws-1-us-west-2, which an aws-0- assumption gets wrong. migration_drift_report.py answers the question you must answer before applying a backlog to an environment that has been touched outside the repo (#317): is the ledger merely BEHIND, or is it LYING? It reports pending files, orphans (recorded here but absent from the repo — flagging filename NUMBER COLLISIONS, the dangerous shape), and any object a pending migration would create that already exists, noting whether that migration is IF NOT EXISTS-safe or would fail the whole run. Object lists are parsed from the migration SQL itself, so there is nothing to keep in sync by hand. Read-only: runs no DDL, safe against production. Verified against a real database both ways — clean (0 pending, 0 orphans, "behind, not lying") and with staging's shape simulated (unrecorded migrations plus a colliding orphan), where it correctly names the collision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A UNIQUE index is the one thing IF NOT EXISTS cannot make safe: it still fails
if the rows already present violate it. That is a DATA problem, invisible to a
schema diff, and it is what turns a clean-looking backlog into a half-applied
run partway through.
The report now parses pending migrations for CREATE UNIQUE INDEX (including the
partial-index WHERE clause) and runs the equivalent GROUP BY ... HAVING count>1
against the live table, naming the offending rows. Generic — it follows
whatever happens to be pending rather than hardcoding today's case.
Verified both directions against a real database: clean data reports none, and
an injected duplicate is caught with the row identified
(0036_offering_null_section_unique -> ('rich-course-math210','summer-2026',2)).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>…I sweep Self-review of this PR found four things, all of which undercut the PR's own premise that the next person shouldn't have to re-derive any of this. pooler_url.py double-encoded the password. urlparse() returns it STILL percent-encoded, and quoting again turned `p%40ss` into `p%2540ss`, so the URI authenticated as the literal escape text. It fails as "password authentication failed" — indistinguishable from simply holding the wrong secret, which is the expensive kind of wrong. Supabase generates passwords with reserved characters, so this was not hypothetical; it was waiting for the next rotation. Decode then re-encode, and pin it with tests, because nothing about the output looks wrong until you try to connect. The workflow's skip-notice hardcoded `aws-0-<region>` while pooler_url.py, added in the same PR, calls that a guess and records that staging is on `aws-1-`. Verified against both live projects: staging answers only on aws-1-us-west-2, production only on aws-0-us-west-2, same region. An operator copying the notice got "Tenant or user not found" — the exact failure class this PR exists to delete. The notice now points at the dashboard and at the builder script. db/migrate.py still told operators the opposite of the PR. Its docstring said "the direct connection string, NOT the pooler" and main()'s unset-variable error routed the reader to Connection string -> Direct. This PR's own repro was running `python -m db.migrate`, so that was the one path left misdocumented. The docstring now explains why the old warning existed (it is still right about transaction mode / 6543) and why it no longer decides the answer (the direct host went IPv6-only). Same correction in CLAUDE.md, README.md, and docs/staging/setup-checklist.md — the checklist being the document someone actually follows to set staging up. Two smaller things while in the same file: the drift report's docstring said "Three sections" after a fourth was added, and main() returned 0 even while printing orphans or data blockers. That second one is a trap for the obvious next refactor — having the workflow call this script instead of duplicating its preflight — which would have silently downgraded a fail-on-orphan gate into a report nobody checks. It now exits 1 on orphans, a non-idempotent collision, or a data blocker; PENDING alone stays clean, since being behind is not drift. Also folds in the one finding from #509's review that cleared review but landed after merge: CLAUDE.md's Commands section still said "add a new numbered file", which #509's own CI guard now rejects. Verification: 1550 passed, 38 skipped (8 new). ruff clean. Drift report re-run against live staging returns exit 1 and correctly names the 3 orphans. No request-path or schema change, so the e2e lanes have nothing to exercise here; the ledger reconciliation that follows will take the full cycle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
28103e2 to
7d31c5dCompareThere was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/scripts/migration_drift_report.py`:
- Line 56: Update RE_INDEX to optionally match the CONCURRENTLY keyword between
the optional IF NOT EXISTS clause and the index name, while preserving existing
CREATE INDEX and CREATE UNIQUE INDEX forms.
In `@backend/tests/test_pooler_url.py`:
- Around line 73-78: Update pooler_host() and
test_a_bare_region_assumes_cluster_zero so bare-region inputs such as
"us-west-2" are rejected instead of defaulting to cluster zero. Require either
an explicit cluster-prefixed region or a complete pooler host, and preserve
valid handling for those supported forms.
In `@docs/staging/setup-checklist.md`:
- Line 26: Update the Step 6 schema migration command in the staging checklist
to run from the backend directory before invoking the db.migrate module, while
preserving the existing SUPABASE_DB_URL assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f6f866a5-e729-40d9-a589-1d1fee5b19d5
📒 Files selected for processing (8)
.github/workflows/migrate-staging.ymlCLAUDE.mdREADME.mdbackend/db/migrate.pybackend/scripts/migration_drift_report.pybackend/scripts/pooler_url.pybackend/tests/test_pooler_url.pydocs/staging/setup-checklist.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/migrate-staging.yml
- backend/scripts/pooler_url.py
| r"alter\s+table\s+([a-z0-9_.]+)\s+add\s+column\s+(?:if\s+not\s+exists\s+)?([a-z0-9_]+)", | ||
| re.I, | ||
| ) | ||
| RE_INDEX = re.compile(r"create\s+(?:unique\s+)?index\s+(?:if\s+not\s+exists\s+)?([a-z0-9_]+)", re.I) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -n -i --glob '*.sql' \
'create\s+(unique\s+)?index\s+concurrently\b' \
backend/db/migrationsRepository: SaplingLearn/Sapling
Length of output: 158
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Inspect the regex usage and migration files of interest without executing repo code.
wc -l backend/scripts/migration_drift_report.py
sed -n '1,120p' backend/scripts/migration_drift_report.py
printf'\nSQL migration files creating indexes:\n'
rg -n -i --glob '*.sql''create\s+(?:unique\s+)?index\b' backend/db/migrations | head -50Repository: SaplingLearn/Sapling
Length of output: 10931
🌐 Web query:
PostgreSQL CREATE INDEX CONCURRENTLY syntax grammar
💡 Result:
In PostgreSQL, the CONCURRENTLY option is used with the CREATE INDEX command to build an index without locking out concurrent writes (inserts, updates, or deletes) to the table [1][2]. The syntax grammar for creating an index concurrently is [1][2]: CREATE [ UNIQUE ] INDEX CONCURRENTLY [ [ IF NOT EXISTS ] name ] ON [ ONLY ] table_name [ USING method ] ( { column_name | ( expression) } [ COLLATE collation ] [ opclass [ ( opclass_parameter = value [,... ]) ] ] [ ASC | DESC ] [ NULLS { FIRST | LAST } ] [,...]) [ INCLUDE ( column_name [,...]) ] [ NULLS [ NOT ] DISTINCT ] [ WITH ( storage_parameter [= value] [,... ]) ] [ TABLESPACE tablespace_name ] [ WHERE predicate ] Key considerations for this command include: 1. Operational Impact: While it avoids blocking writes, the command requires two scans of the table and must wait for existing transactions that could modify or use the index to terminate [1][2]. 2. Performance: It requires more total work and takes significantly longer to complete than a standard index build [1][2]. It also imposes extra CPU and I/O load on the database [1][2]. 3. Failure Recovery: If the build fails (e.g., due to a deadlock or a duplicate key violation), the index is left in an invalid state. The recommended recovery method is to drop the invalid index and retry the CREATE INDEX CONCURRENTLY command [2].
Citations:
- 1: https://www.postgresql.org/docs/current/sql-createindex.html
- 2: https://www.postgresql.org/docs/19/sql-createindex.html
🌐 Web query:
PostgreSQL CREATE INDEX CONCURRENTLY IF NOT EXISTS supported since version grammar
💡 Result:
The syntax CREATE INDEX CONCURRENTLY IF NOT EXISTS is supported in PostgreSQL version 10 and all subsequent versions [1]. The IF NOT EXISTS clause was introduced for the CREATE INDEX command in PostgreSQL 10 to allow index creation without throwing an error if an index with the same name already exists [1]. When using this syntax, the index name is mandatory [2][1]. The CONCURRENTLY option allows the index to be built without locking out concurrent writes to the table [2][3]. As of PostgreSQL 10 and later, the general grammar for this command is: CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] name ] ON table_name [ USING method ] ( { column_name | ( expression) } [... ]) [... ] [2][1]
Citations:
- 1: https://www.postgresql.org/docs/10/sql-createindex.html
- 2: https://www.postgresql.org/docs/19/sql-createindex.html
- 3: https://www.postgresql.org/docs/11/sql-createindex.html
Support CREATE INDEX CONCURRENTLY in RE_INDEX.
RE_INDEX ignores CONCURRENTLY before the index name. When a pending migration uses the valid CREATE INDEX CONCURRENTLY ... form, the report can skip its existing-index collision check and return clean while the migration still fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/migration_drift_report.py` at line 56, Update RE_INDEX to
optionally match the CONCURRENTLY keyword between the optional IF NOT EXISTS
clause and the index name, while preserving existing CREATE INDEX and CREATE
UNIQUE INDEX forms.
| def test_a_bare_region_assumes_cluster_zero(self): | ||
| """Documented as a guess, not a derivation — kept so a bare region is | ||
| still usable, but it is why the dashboard is the real source.""" | ||
| from scripts.pooler_url import pooler_host | ||
| assert pooler_host("us-west-2") == "aws-0-us-west-2.pooler.supabase.com" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject bare regions instead of selecting cluster zero.
The cluster number is not derivable from the region. This test locks pooler_host() into generating an aws-0-... host for every bare-region input. A project on another cluster, such as aws-1-us-west-2, gets an invalid migration endpoint. Require an explicit cluster prefix or complete pooler host, and make bare-region input fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_pooler_url.py` around lines 73 - 78, Update pooler_host()
and test_a_bare_region_assumes_cluster_zero so bare-region inputs such as
"us-west-2" are rejected instead of defaulting to cluster zero. Require either
an explicit cluster-prefixed region or a complete pooler host, and preserve
valid handling for those supported forms.
| `python -c "import secrets; print(secrets.token_hex(32))"` | ||
| - [ ] Create storage buckets `avatars` and `cosmetic-assets` (match prod). | ||
| - [ ] Once you have the keys locally (Step 6), apply schema: `SUPABASE_DB_URL=<direct> python -m db.migrate`. | ||
| - [ ] Once you have the keys locally (Step 6), apply schema: `SUPABASE_DB_URL=<session-pooler> python -m db.migrate`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Run this command from backend/.
The checklist uses root-relative paths later in the workflow. From the repository root, python -m db.migrate cannot resolve the db package. Change this command to enter backend/ before invoking the module.
Proposed fix
-- [ ] Once you have the keys locally (Step 6), apply schema: `SUPABASE_DB_URL=<session-pooler> python -m db.migrate`.+- [ ] Once you have the keys locally (Step 6), apply schema: `cd backend && SUPABASE_DB_URL=<session-pooler> python -m db.migrate`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -[ ] Once you have the keys locally (Step 6), apply schema: `SUPABASE_DB_URL=<session-pooler> python -m db.migrate`. | |
| -[ ] Once you have the keys locally (Step 6), apply schema: `cd backend && SUPABASE_DB_URL=<session-pooler> python -m db.migrate`. |
🤖 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 `@docs/staging/setup-checklist.md` at line 26, Update the Step 6 schema
migration command in the staging checklist to run from the backend directory
before invoking the db.migrate module, while preserving the existing
SUPABASE_DB_URL assignment.
Follow-up to #506, found by actually trying to run the migration.
What happened
venv/bin/dotenv -f .env.staging run -- venv/bin/python -m db.migratefails:Diagnosis — not credentials, not the command:
Canopy already recorded this from the #481 work — "direct psycopg to staging is blocked — IPv6-only endpoint refuses" — so it's a known wall, and I walked into it anyway.
Why this is a defect in #506, not just an operator note
The workflow's skip notice said to use "the DIRECT connection string, port 5432, not the pooler."GitHub-hosted runners have no outbound IPv6, so its first real run would have failed identically — and the message I wrote would have sent whoever debugged it back to the same host.
The fix
Pooler hosts publish A records (
aws-0-us-east-1.pooler.supabase.com → 44.208.221.186), so:postgres.<ref>, easy to miss.On
db/migrate.py's "SUPABASE_DB_URL, NOT the pooler" — I'm contradicting that deliberately rather than quietly. That warning is about transaction mode and predates the IPv6-only endpoint; session mode is the correct substitute, and with the direct host unreachable it's the only one. Said so in the file so the next person doesn't have to re-derive it.Docs only — header rationale and the skip notice. No behaviour change.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation