fix(admin): Supabase drift report + close admin-rollup 42501 root cause + drift guard - #775
Merged
Merged
Conversation
#732, #772) Re-verifies the Bucket A Supabase drift findings from the stabilization brief directly against live information_schema/pg_proc (not migration history, per the brief's own guardrail). All of #651's 12 columns, #728's function body, and #772's two linked-lint functions are already resolved in production as of this pass. #732 has no first-party public.rate_limits reference; reclassified as needing log correlation instead of a schema fix. Also documents a new, separate finding: local migration filenames and the remote-applied version numbers are systemically mismatched for essentially every migration since ~2026-05-26 (193 local-only / 445 remote-only by version, almost entirely 1:1 name-paired), and npm run check:ledger is not wired into any CI job. Read-only pass only — no migrations created/applied, no db push/repair/reset run. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughAdds a read-only Supabase drift report, a drift-check CLI and runbook, and a migration that updates admin rollup authorization and self-role-change behavior. ChangesSupabase Drift Verification
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
eslint.config.mjsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. scripts/db/check-supabase-drift.mjsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
PR Summary by Qododocs(audits): add read-only Supabase drift verification report (2026-07-03)
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
97 rules 1. Wrong npm script name
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+191
to
+193
| the fact without a corresponding ledger update. Either way, **`npm run | ||
| check:migration-ledger` (if wired into CI) would fail continuously** | ||
| on this repo in its current state, and `supabase migration list |
There was a problem hiding this comment.
1. Wrong npm script name 🐞 Bug ⚙ Maintainability
The report mentions npm run check:migration-ledger, but the repo only defines check:ledger, so the referenced command will fail if someone tries to run it to reproduce the ledger check.
Agent Prompt
## Issue description
The drift report references an npm script `check:migration-ledger` that is not defined in `package.json`. This creates a docs/repro mismatch (the actual script is `check:ledger`).
## Issue Context
`package.json` defines `check:ledger` pointing at `scripts/check-migration-ledger.mjs`, but there is no `check:migration-ledger` script.
## Fix Focus Areas
- docs/audits/SUPABASE_DRIFT_REPORT_2026-07-03.md[191-193]
- package.json[5-9]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…ift guard (#736 follow-up) While building the Phase 4 drift guard, direct inspection of live admin RPCs found that PR #736 fixed the symptom (restored the admin row, added admin_allowlist/is_super_admin()) but never migrated the RPC layer off the original bug: all 11 get_admin_*_rollup SECURITY DEFINER functions (6 directly, 5 via the shared __admin_rollup_b_gate() helper) still gate solely on `users.role = 'admin'`. Bridge's app-layer gate (requireSuperAdmin()) only checks SUPER_ADMIN_USER_IDS + admin_allowlist and never looks at users.role, so the original failure mode -- "requireSuperAdmin() still passed, but admin panel RPCs died with 42501" -- is still fully reproducible today by anything that flips users.role for the allowlisted admin, not just the specific baseball-onboarding path already patched. Also found: guard_users_role_self_change() blocked self-*escalation* to a role outside player/coach, but explicitly allowed 'coach'/'player' as NEW.role -- meaning it did nothing to stop a super admin's own onboarding/profile-update flow from silently self-demoting from admin to coach, which is the literal #736 incident. That specific transition was never actually blocked. Migration `admin_rollup_consistent_super_admin_gate` (applied to production and verified live): - Every rollup gate now accepts EITHER public.is_super_admin() OR the legacy users.role = 'admin' check -- additive only, no currently authorized caller loses access (verified pre-change that admin_allowlist and users.role='admin' are the same single user). - guard_users_role_self_change() now also blocks OLD.role = 'admin' -> NEW.role IN (coach, player) for allowlisted super admins. New: scripts/db/check-supabase-drift.mjs (`npm run db:drift:check`) -- read-only guard covering #651 columns, golf drift shape, #728 function body, #772 linked-lint functions, #732 rate_limits, admin rollup RPC existence + consistent gating, and the role-demotion guard. Queries information_schema/pg_proc directly rather than schema_migrations, per docs/audits/SUPABASE_DRIFT_REPORT_2026-07-03.md's finding that the migration ledger cannot be trusted alone on this project. Co-authored-by: Cursor <cursoragent@cursor.com>
njrini99-code
changed the title
coderabbitai
Bot
added
ci
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 `@scripts/db/check-supabase-drift.mjs`:
- Around line 151-170: The super-admin-gate drift check is matching raw SQL
text, so comment-only mentions of is_super_admin can incorrectly satisfy it.
Extract the existing comment-stripping behavior from
can_manage_baseball_lift_group into a shared helper and reuse it in the admin
rollup RPCs check so the match only considers executable SQL, not -- comment
lines.
- Around line 26-34: The drift check script builds its connection string from
process.env before loading local env files, so it misses values from .env.local.
Update the entrypoint in check-supabase-drift.mjs to load dotenv/config (or
equivalent) before buildConnectionString() reads DATABASE_URL,
SUPABASE_PROJECT_ID, and SUPABASE_DB_PASSWORD, matching the check:stats
convention and ensuring .env.local is consulted first.
- Around line 26-34: The connection string builder in buildConnectionString()
interpolates SUPABASE_DB_PASSWORD directly into the URL, so passwords with
reserved URL characters can break parsing. Update the password handling in
buildConnectionString() to URL-encode the password before composing the
postgresql://postgres.${projectId}:...@${POOLER_HOST}:6543/postgres string,
while keeping the DATABASE_URL passthrough unchanged.
- Around line 27-29: The script uses Node globals like process and console in
check-supabase-drift.mjs, but the ESLint setup does not currently treat
scripts/**/*.mjs as Node code. Update the shared ESLint config in
eslint.config.mjs to add a Node globals/override for scripts/**, or add a local
/* eslint-env node */ annotation in this script if you want the scope to stay
isolated. Make sure the fix covers the getDatabaseUrl logic and the later
logging/exit usage in the main drift-check flow.
- Line 312: The Supavisor transaction-mode connection created in the drift check
script is using the default prepared statements, which can cause intermittent
failures. Update the postgres client initialization in the connection setup to
disable prepared statements by adding the appropriate option alongside ssl and
max, and keep this change in the code path that builds the Supabase pooler
connection.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 853a486a-5220-4c15-9272-4c59380ade5f
📒 Files selected for processing (4)
docs/operations/SUPABASE_DRIFT_GUARD.mdpackage.jsonscripts/db/check-supabase-drift.mjssupabase/migrations/20260703210000_admin_rollup_consistent_super_admin_gate.sql
This was referenced Jul 3, 2026
|
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. |
njrini99-code
added a commit
that referenced
this pull request
Jul 3, 2026
Phase 14 of the HelmV3 stabilization brief — final deliverable. Classifies every issue named in the brief against evidence gathered this session. No issues closed automatically (recommendations only, per the brief's guardrail). Headline findings: - #516 (hardcoded service-role key) was marked closed but was NOT actually fixed — reopened in spirit via PR #774, key rotation is still an outstanding manual action. - #651, #728, and both #772 linked-lint findings verified resolved directly against production. - #736's "prevent recurrence" criterion was not actually met until this pass (PR #775) — the RPC layer never migrated off the gate model that caused the original incident. - #390 and #388 (CI runbook issues) were already fully closed and verified accurate — no new work needed. - #477, #442/#443, #415 gained regression coverage this pass (PR #776); #406 and true #442 concurrency are blocked on Docker not being available in this environment. Full PR list, dependency-PR triage (2 merged, 1 blocked on a real Review Gate failure, 6 left for individual review), and everything explicitly out of scope are listed at the bottom. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Aug 1, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related pieces from the HelmV3 stabilization brief, landed together because the second was discovered while building the drift guard needed for the first:
1. Read-only Supabase drift verification report
Re-verifies the Bucket A Supabase drift findings (#651, #728, #732, #772 linked-lint) directly against live
information_schema/pg_procvia the Supabase MCP, per the brief's instruction not to trustschema_migrationsalone.recalculate_baseball_season_statsb.sodrift) — nob.soreference in the live body; now includes a coach-authorization guard. Resolved.can_manage_baseball_lift_group,baseball_accept_staff_invite) — neither references the stale relations/fields. Resolved.public.rate_limits/expires_at) —public.rate_limitsdoesn't exist; reclassified as needing log correlation, not a schema fix.npm run check:ledgerexists but is wired into no CI job.2. Admin rollup 42501 root cause (PR #736 follow-up) + Supabase drift guard
While building the drift guard, found that PR #736 fixed the symptom of the admin-panel outage (restored the admin row, added
admin_allowlist/is_super_admin()) but never migrated the RPC layer off the bug that caused it:get_admin_*_rollupSECURITY DEFINER functions (6 directly, 5 via the shared__admin_rollup_b_gate()helper) still gated solely onusers.role = 'admin'. Bridge's app-layer gate (requireSuperAdmin()) only checksSUPER_ADMIN_USER_IDS+admin_allowlistand never looks atusers.role— so the original failure ("requireSuperAdmin()still passed, but admin panel RPCs died with 42501") was still fully reproducible by anything that flipsusers.role, not just the specific baseball-onboarding path already patched.guard_users_role_self_change()blocked self-escalation to a role outside player/coach, but explicitly allowed'coach'/'player'asNEW.role— i.e. it did nothing to stop a super admin's own onboarding/profile-update flow from silently self-demotingadmin→coach, which is the literal P0: stop baseball onboarding demoting admins + honest Bridge panel fallback #736 incident. That specific transition was never actually blocked.Migration
admin_rollup_consistent_super_admin_gate(applied to production, verified live viapg_get_functiondef):public.is_super_admin()OR the legacyusers.role = 'admin'check — additive only; verified pre-change thatadmin_allowlistandusers.role='admin'are the same single user, so no currently authorized caller loses access.guard_users_role_self_change()now also blocksOLD.role = 'admin'→NEW.role IN (coach, player)for allowlisted super admins.New:
scripts/db/check-supabase-drift.mjs(npm run db:drift:check) — read-only guard covering #651 columns, golf drift shape, #728 function body, #772 linked-lint functions, #732 rate_limits, admin rollup RPC existence + consistent gating, and the role-demotion guard. Queriesinformation_schema/pg_procdirectly rather thanschema_migrations, per the drift report's finding that the migration ledger can't be trusted alone here.Also surfaced (documented, not fixed in this PR)
npm run test:rls(vitest --project rls) matches zero files under its own*.rls.test.{ts,tsx}glob — there are no RLS-specific test files anywhere in the repo, so that CI lane silently runs generic unit tests under the "rls" label instead of testing RLS policies. Same "safety net that never actually runs" pattern as the deadscripts/__tests__/secrets guard fixed in fix(security): remove live hardcoded service-role secrets from scripts (#516) #774.What this PR does NOT do
supabase db push,migration repair, ordb reset.test:rlsgap or the migration-ledger mismatch — both documented for a dedicated follow-up.Test plan
apply_migration, approved via the native approval card (sensitive admin-RPC mutation).__admin_rollup_b_gate+ 6 rollups +guard_users_role_self_change) now referenceis_super_admin().npm run test:rls(4472 passed / 39 skipped) andnpm run test:run(4398 passed / 39 skipped) — both green post-migration.npm run typecheckclean.src/lib/types/database.tsregenerated by the pre-commit hook — diff was empty (function body changes don't affect generated types).Related