Uh oh!
There was an error while loading. Please reload this page.
chore(db): staging ops scripts + newsletter_emails.approved_at migration - #261
chore(db): staging ops scripts + newsletter_emails.approved_at migration#261AndresL230 wants to merge 3 commits into
Conversation
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | b08e2bd | Commit Preview URL Branch Preview URL | Jun 24 2026, 04:56 AM |
Warning Review limit reached
More reviews will be available in 42 minutes and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between d4d0862fac6b028543982c7c6407c42abc683c86 and b08e2bd. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a shared Staging Operational Scripts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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/db/_staging_ops.py`:
- Around line 39-49: The confirm_write function currently only prints the target
host and returns the apply flag without validating that the database URL is
actually safe for writing. Add a hard safety guard that validates the database
URL points to a staging or safe environment before the function returns apply as
True. Check the target using the target_host function or similar mechanism to
determine if it is a production database, and either reject the write operation
entirely or require an explicit force flag parameter to bypass the safety check
when attempting to write to non-staging targets.
- Around line 61-67: The index dictionary in the function is silently
overwriting entries when duplicate plaintext emails are encountered, which can
cause downstream operations to target the wrong user ID. Before assigning to the
index dictionary on the line index[plain] = (uid, approved), add a check to
detect if that plaintext email key already exists in the index. If a duplicate
is found, raise an exception with a clear error message indicating the collision
and requiring manual disambiguation, rather than allowing the silent overwrite
to occur.
- Around line 4-6: The comment in _staging_ops.py documents a deliberate bypass
of the standard db/connection.py::table() requirement for cross-project staging
operations, but this exception is not formalized in the coding guidelines.
Update the coding guidelines documentation (the rule about "All Supabase access
must go through backend/db/connection.py::table()") to add a path-scoped
exception that explicitly permits direct Supabase access in the staging
operation scripts (backend/db/migrate.py, backend/db/approve_staging_users.py,
backend/db/grant_staging_admin.py, and backend/db/copy_courses_to_staging.py).
This codification will ensure future maintainers understand when and why the
bypass is acceptable.
🪄 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: 2903be73-dde8-4d23-9afc-d0bc63f8c175
📥 Commits
Reviewing files that changed from the base of the PR and between f932cd3 and d4d0862fac6b028543982c7c6407c42abc683c86.
📒 Files selected for processing (5)
backend/db/_staging_ops.pybackend/db/approve_staging_users.pybackend/db/copy_courses_to_staging.pybackend/db/grant_staging_admin.pybackend/db/migrations/0019_newsletter_approved_at.sql
| Those scripts span two Supabase projects and so deliberately bypass | ||
| db/connection.py::table() (which is single-environment) — see each script's module | ||
| docstring. The common bits live here to keep them from diverging: dotenv reading, a |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Verify the scope of direct database/http clients in backend/db scripts.
fd -e py . backend/db | xargs rg -n "psycopg\.connect|httpx\.Client|from supabase|import supabase"Repository: SaplingLearn/Sapling
Length of output: 741
🏁 Script executed:
cat -n backend/db/_staging_ops.py | head -50Repository: SaplingLearn/Sapling
Length of output: 2465
🏁 Script executed:
head -70 backend/db/approve_staging_users.pyRepository: SaplingLearn/Sapling
Length of output: 2878
Codify the exception for multi-project staging operations in the coding guidelines, or refactor to use backend/db/connection.py::table().
Lines 4–6 document a deliberate bypass of db/connection.py::table() for cross-project staging operations. This conflicts with the **/*.py guideline: "All Supabase access must go through backend/db/connection.py::table(). Do not instantiate httpx clients or import supabase directly elsewhere."
While the bypass is justified (these scripts span two Supabase projects), the exception must be codified in the coding guidelines as a path-scoped rule for backend/db/{migrate,approve_staging_users,grant_staging_admin,copy_courses_to_staging}.py rather than left as an undocumented practice. This ensures future maintainers understand when and why direct database access is acceptable.
🤖 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/db/_staging_ops.py` around lines 4 - 6, The comment in
_staging_ops.py documents a deliberate bypass of the standard
db/connection.py::table() requirement for cross-project staging operations, but
this exception is not formalized in the coding guidelines. Update the coding
guidelines documentation (the rule about "All Supabase access must go through
backend/db/connection.py::table()") to add a path-scoped exception that
explicitly permits direct Supabase access in the staging operation scripts
(backend/db/migrate.py, backend/db/approve_staging_users.py,
backend/db/grant_staging_admin.py, and backend/db/copy_courses_to_staging.py).
This codification will ensure future maintainers understand when and why the
bypass is acceptable.
Source: Coding guidelines
| def confirm_write(db_url: str, apply: bool, action: str) -> bool: | ||
| """Safe-by-default guard against pointing a write at the wrong project. | ||
| Prints the destination host. Returns True only when --yes (apply=True) was | ||
| passed; otherwise prints a preview notice and returns False so the caller can | ||
| report what it *would* do without mutating anything. | ||
| """ | ||
| print(f"Write target: {target_host(db_url)}") | ||
| if not apply: | ||
| print(f" PREVIEW ONLY — re-run with --yes to {action}. No changes made.") | ||
| return apply |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
confirm_write is preview gating, not actual target validation.
Line 46 only prints the host, and Line 49 returns apply unchanged. With --yes, a mispointed DB URL can still write to the wrong project. Add a hard non-staging guard (or explicit force flag) before any mutation path runs.
🤖 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/db/_staging_ops.py` around lines 39 - 49, The confirm_write function
currently only prints the target host and returns the apply flag without
validating that the database URL is actually safe for writing. Add a hard safety
guard that validates the database URL points to a staging or safe environment
before the function returns apply as True. Check the target using the
target_host function or similar mechanism to determine if it is a production
database, and either reject the write operation entirely or require an explicit
force flag parameter to bypass the safety check when attempting to write to
non-staging targets.
| cur.execute("SELECT id, email, is_approved FROM users") | ||
| index: dict[str, tuple[str, bool]] = {} | ||
| for uid, email_ct, approved in cur.fetchall(): | ||
| plain = (decrypt_if_present(email_ct) or "").strip().lower() | ||
| if plain: | ||
| index[plain] = (uid, approved) | ||
| return index |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Duplicate plaintext emails are silently overwritten in the index.
Line 66 replaces existing entries for the same normalized email. If duplicates exist, downstream approval/admin actions can target the wrong user id. Fail fast on collisions and require manual disambiguation.
Suggested guard
def user_email_index(cur) -> dict[str, tuple[str, bool]]:
@@
for uid, email_ct, approved in cur.fetchall():
plain = (decrypt_if_present(email_ct) or "").strip().lower()
if plain:
- index[plain] = (uid, approved)+ existing = index.get(plain)+ if existing and existing[0] != uid:+ raise SystemExit(+ f"ERROR: duplicate decrypted email {plain!r} maps to multiple user ids "+ f"({existing[0]} and {uid}). Resolve before continuing."+ )+ index[plain] = (uid, approved)📝 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.
| cur.execute("SELECT id, email, is_approved FROM users") | |
| index: dict[str, tuple[str, bool]] = {} | |
| foruid, email_ct, approvedincur.fetchall(): | |
| plain= (decrypt_if_present(email_ct) or"").strip().lower() | |
| ifplain: | |
| index[plain] = (uid, approved) | |
| returnindex | |
| cur.execute("SELECT id, email, is_approved FROM users") | |
| index: dict[str, tuple[str, bool]] = {} | |
| foruid, email_ct, approvedincur.fetchall(): | |
| plain= (decrypt_if_present(email_ct) or"").strip().lower() | |
| ifplain: | |
| existing=index.get(plain) | |
| ifexistingandexisting[0] !=uid: | |
| raiseSystemExit( | |
| f"ERROR: duplicate decrypted email {plain!r} maps to multiple user ids " | |
| f"({existing[0]} and {uid}). Resolve before continuing." | |
| ) | |
| index[plain] = (uid, approved) | |
| returnindex |
🤖 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/db/_staging_ops.py` around lines 61 - 67, The index dictionary in the
function is silently overwriting entries when duplicate plaintext emails are
encountered, which can cause downstream operations to target the wrong user ID.
Before assigning to the index dictionary on the line index[plain] = (uid,
approved), add a check to detect if that plaintext email key already exists in
the index. If a duplicate is found, raise an exception with a clear error
message indicating the collision and requiring manual disambiguation, rather
than allowing the silent overwrite to occur.
Add three idempotent, safe-by-default (preview unless --yes) ops scripts used to seed staging from prod and bootstrap access, plus a shared helper: - copy_courses_to_staging.py — copy the plaintext `courses` catalog prod→staging (prod via PostgREST, staging via psycopg; additive upsert on id, no deletes). - grant_staging_admin.py — approve + grant the admin role, matched by decrypted email; admin role_id looked up by slug (per-project UUID). - approve_staging_users.py — approve accounts (is_approved) by decrypted email; reports emails with no staging account yet as PENDING. - _staging_ops.py — shared dotenv reader, staging-key setup, decrypt-email→user index, and a write-target guard. These deliberately bypass db/connection.py::table() (single-environment) since they span two Supabase projects, mirroring db/migrate.py's direct-connection exception. Credentials are read from gitignored dotenv files, never argv/process env. Also add migration 0019: newsletter_emails.approved_at. The admin allowlist endpoints read/write this column, but it was added to prod out-of-band and never captured as a migration, so staging/CI/fresh DBs were missing it (500s the endpoint). ADD COLUMN IF NOT EXISTS is a no-op where it already exists (prod). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d4d0862 to
8718c1fCompareReplace the len(batch) < PAGE termination, which under-fetches silently when prod's db-max-rows is below PAGE (the first response is short and the loop stops early). Request pages with the Range header plus Prefer: count=exact and loop until offset reaches the total reported in Content-Range.
created_at is effectively immutable; excluding it from the ON CONFLICT DO UPDATE SET clause prevents re-runs from replacing staging's existing value with prod's. It stays in the INSERT column list for first seed.
AndresL230
commented
Jun 25, 2026
The |
What
Tooling used to seed the new staging environment from prod and bootstrap access, plus a schema-drift fix surfaced along the way.
Ops scripts (
backend/db/)Three idempotent, safe-by-default scripts (print the write target and preview unless
--yes), sharing a small helper:copy_courses_to_staging.pycoursescatalog prod→staging (prod via PostgREST, staging via psycopg). Additive upsert onid— no deletes.grant_staging_admin.py <email>adminrole, matched by decrypted email. Adminrole_idlooked up byslug(per-project UUID).approve_staging_users.py <emails...>is_approved) by decrypted email; reports emails with no staging account yet as PENDING._staging_ops.pyThese deliberately bypass
db/connection.py::table()(single-environment) since they span two Supabase projects — the same exceptiondb/migrate.pyalready makes. Credentials are read from gitignored dotenv files, never argv/process env.Migration
0019_newsletter_approved_at.sqlnewsletter_emails.approved_atexists in prod (added out-of-band) but was never captured as a migration, so any DB built from migrations (staging, CI, fresh) was missing it — which 500s the admin allowlist endpoints (/api/admin/allowlist/approve|revoke).ADD COLUMN IF NOT EXISTSmakes it a no-op where it already exists.Verification
0019applied to staging;approved_atconfirmed present.db.ybgq…, zero writes).ruff checkpasses;ruff formatapplied.Follow-up (not in this PR)
0019on prod so itsschema_migrationsledger matches reality (no-op column-wise; needs prod's direct DB connection, which isn't in.env).main's Frontend CI is currently red repo-wide (lockfile / eslint baseline) — unrelated to this backend-only change.🤖 Generated with Claude Code
Summary by CodeRabbit
Database Changes
Chores