P0: stop baseball onboarding demoting admins + honest Bridge panel fallback - #736
Merged
Conversation
…allback with retry [vercel skip]
P0 from the 2026-07-03 Mission Control sweep. Root cause (H1 confirmed):
every get_admin_*_rollup SECURITY-DEFINER function gates on
users.role='admin', but baseball onboarding's users upsert
({ onConflict: 'id' }, service-role client, no ignoreDuplicates) clobbered
the allowlisted super-admin's row down to role='coach' — so
requireSuperAdmin() passed and every Bridge panel then died with 42501.
The role was restored live (2026-07-03 04:18Z, verified: simulated
authenticated calls to get_admin_dashboard_rollup/get_admin_rounds_rollup
return data); this commit makes the demotion impossible to reintroduce:
- ensureUserRowPreservingAdmin(): both baseball onboarding users-row writes
now insert-if-missing / allow player<->coach conversion / NEVER overwrite
an existing 'admin'. (Golf onboarding already used ignoreDuplicates and
could not demote.)
- PanelBoundary/PanelStale: the error fallback claimed "showing last known
data" while rendering none. Copy now tells the truth ("temporarily
unavailable") and ships a real retry (router.refresh() + boundary remount).
- panel-boundary tests: assert the honest copy, the retry path, and that the
misleading claim never comes back.
H2 (session not reaching the RPC) ruled out: fetchAdminRollupA uses the
request-scoped server client and the RPC succeeds with only a sub claim.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fz1ajgmqcEp1yPEzjnXgY5
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
WalkthroughThis PR adds a retry mechanism to admin panel error boundaries (PanelRetryButton triggering router.refresh and boundary remount via attempt-key state), updates PanelStale's copy and action prop, adds corresponding tests, and introduces an ensureUserRowPreservingAdmin helper in onboarding to prevent role upserts from demoting existing admin users. ChangesPanel Retry Flow
Estimated code review effort: 2 (Simple) | ~12 minutes Admin-Preserving Onboarding Role Updates
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PanelRetryButton
participant Router as next/navigation Router
participant PanelErrorBoundary
participant PanelBoundary
User->>PanelRetryButton: click "Try again"
PanelRetryButton->>Router: refresh()
PanelRetryButton->>PanelErrorBoundary: reset()
PanelErrorBoundary->>PanelErrorBoundary: clear stored error
PanelErrorBoundary->>PanelBoundary: onRetryReset()
PanelBoundary->>PanelBoundary: attempt++ (remount via key)
PanelBoundary->>PanelErrorBoundary: remount with refreshed children
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (9 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. 🔧 ast-grep (0.44.0)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml Comment |
PR Summary by QodoPrevent onboarding from demoting admins; add honest admin panel retry fallback
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb302776be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const retry = useCallback(() => { | ||
| startTransition(() => { | ||
| router.refresh(); | ||
| onReset(); |
There was a problem hiding this comment.
For the async server panels this boundary wraps, router.refresh() only schedules a new RSC payload; calling onReset() immediately re-renders the same throwing children before that payload has arrived, putting the new keyed boundary back into its error state. When the refreshed payload later succeeds, the boundary still renders the fallback because its error state is already set, so a transient admin-panel RPC/Sentry failure can remain stuck on “temporarily unavailable” until a full page reload.
Useful? React with 👍 / 👎.
Code Review by Qodo
Context used✅ Compliance rules (platform):
93 rules 1. src/app/admin missing registry mapping
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+73
to
+88
| const { data: existing, error: readError } = await admin | ||
| .from('users') | ||
| .select('role') | ||
| .eq('id', userId) | ||
| .maybeSingle(); | ||
| if (readError) return { error: readError }; | ||
|
|
||
| if (!existing) { | ||
| const { error } = await admin | ||
| .from('users') | ||
| .upsert({ id: userId, email, role }, { onConflict: 'id', ignoreDuplicates: true }); | ||
| return { error }; | ||
| } | ||
| if (existing.role === 'admin' || existing.role === role) return { error: null }; | ||
|
|
||
| const { error } = await admin.from('users').update({ role }).eq('id', userId); |
There was a problem hiding this comment.
1. Unprefixed users table access 📘 Rule violation ⚙ Maintainability
The new onboarding helper queries and writes the users table via admin.from('users'), which
violates the required golf_/baseball_ table-prefix convention. This can reintroduce ambiguous
cross-domain table usage and breaks the naming policy enforced by the checklist.
Agent Prompt
## Issue description
New code references the unprefixed `users` table, but compliance requires all table identifiers to start with `golf_` or `baseball_`.
## Issue Context
This was introduced in the new `ensureUserRowPreservingAdmin()` helper.
## Fix Focus Areas
- src/app/baseball/actions/onboarding.ts[73-88]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+28
to
47
| export function PanelStale({ | ||
| label, | ||
| error, | ||
| action, | ||
| }: { | ||
| label: string; | ||
| error?: string; | ||
| action?: React.ReactNode; | ||
| }) { | ||
| // Copy must be honest: this state renders when the fetch FAILED and there | ||
| // is no data below it. It previously claimed "showing last known data", | ||
| // which was never true (nothing cached is rendered) — flagged in the | ||
| // 2026-07-03 Mission Control sweep while every panel was down with 42501. | ||
| return ( | ||
| <div className="flex flex-col items-center gap-2 rounded-xl bg-fw-warning-bg px-6 py-8 text-center"> | ||
| <CloudOff size={20} className="text-fw-warning" aria-hidden /> | ||
| <p className="text-sm font-medium text-warm-800">{label} — showing last known data</p> | ||
| <p className="text-sm font-medium text-warm-800">{label} — temporarily unavailable</p> | ||
| {error ? <p className="font-fw-mono text-xs text-warm-600">{error}</p> : null} | ||
| {action} | ||
| </div> |
There was a problem hiding this comment.
3. panelstale exceeds empty-state spec 📘 Rule violation ✧ Quality
PanelStale renders multiple text blocks (label line plus optional error text) and allows arbitrary extra content via action, exceeding the allowed empty-state structure. This can lead to inconsistent empty-state layouts and copy across the app.
Agent Prompt
## Issue description
The `PanelStale` empty-state component renders extra content beyond the allowed icon/title/one-sentence description/single CTA structure.
## Issue Context
It currently includes an optional additional error paragraph and an `action` slot that can render arbitrary nodes.
## Fix Focus Areas
- src/app/admin/_components/PanelStates.tsx[28-47]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
1
to
6
| 'use client'; | ||
|
|
||
| import { Component, Suspense, type ReactNode } from 'react'; | ||
| import { Component, Suspense, useCallback, useState, useTransition, type ReactNode } from 'react'; | ||
| import { useRouter } from 'next/navigation'; | ||
| import { SkeletonStat } from '@/components/fairway'; | ||
| import { PanelStale } from './PanelStates'; |
There was a problem hiding this comment.
4. src/app/admin missing registry mapping 📘 Rule violation ⚙ Maintainability
The PR modifies admin UI code under src/app/admin/, but memory/registry.yml does not map that route tree to any feature entry. This violates the requirement to update the feature registry when touching unmapped features.
Agent Prompt
## Issue description
Feature registry does not include mappings for the modified `src/app/admin/**` route tree.
## Issue Context
`memory/registry.yml` currently maps admin to `src/app/golf/admin/**`, but nothing covers `src/app/admin/**`.
## Fix Focus Areas
- memory/registry.yml[955-985]
- src/app/admin/_components/PanelBoundary.tsx[1-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+73
to
+89
| const { data: existing, error: readError } = await admin | ||
| .from('users') | ||
| .select('role') | ||
| .eq('id', userId) | ||
| .maybeSingle(); | ||
| if (readError) return { error: readError }; | ||
|
|
||
| if (!existing) { | ||
| const { error } = await admin | ||
| .from('users') | ||
| .upsert({ id: userId, email, role }, { onConflict: 'id', ignoreDuplicates: true }); | ||
| return { error }; | ||
| } | ||
| if (existing.role === 'admin' || existing.role === role) return { error: null }; | ||
|
|
||
| const { error } = await admin.from('users').update({ role }).eq('id', userId); | ||
| return { error }; |
There was a problem hiding this comment.
5. Admin demotion race window 🐞 Bug ⛨ Security
ensureUserRowPreservingAdmin() does a separate read of users.role and then an unconditional UPDATE by id; if the row becomes role='admin' between those statements, the service_role update can still overwrite it back to 'coach'/'player'. This undermines the PR’s goal (preventing admin lockout) because service_role writes bypass the self-escalation trigger guard.
Agent Prompt
### Issue description
`ensureUserRowPreservingAdmin()` is a TOCTOU (read-then-write) implementation. A concurrent promotion to `role='admin'` can still be overwritten by the subsequent `UPDATE`, because the update has no DB-side guard.
### Issue Context
This helper is intentionally run via a service-role client; service-role writes are not blocked by the self-escalation guard trigger.
### Fix Focus Areas
- src/app/baseball/actions/onboarding.ts[60-90]
### Suggested fix
Implement an **atomic** write that preserves admin at the database level:
- Prefer a single SQL statement (via RPC or `upsert` + server-side function) that does `ON CONFLICT (id) DO UPDATE SET role = CASE WHEN users.role='admin' THEN users.role ELSE EXCLUDED.role END`.
- If you keep app-side logic, at minimum guard the update with `.neq('role','admin')` (or equivalent) and handle the “0 rows updated” case by re-reading the role and returning success when it is `admin`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+80
to
+89
| if (!existing) { | ||
| const { error } = await admin | ||
| .from('users') | ||
| .upsert({ id: userId, email, role }, { onConflict: 'id', ignoreDuplicates: true }); | ||
| return { error }; | ||
| } | ||
| if (existing.role === 'admin' || existing.role === role) return { error: null }; | ||
|
|
||
| const { error } = await admin.from('users').update({ role }).eq('id', userId); | ||
| return { error }; |
There was a problem hiding this comment.
7. Users email not refreshed 🐞 Bug ≡ Correctness
ensureUserRowPreservingAdmin() only writes email during the insert path, and its existing-row path either early-returns or updates only role, so public.users.email can become stale. Since the app supports auth email changes and other flows send notifications using public.users.email, this can cause notifications to be sent to the wrong address.
Agent Prompt
### Issue description
When a `users` row already exists, `ensureUserRowPreservingAdmin()` does not update `email` (and when it updates a role it still only updates `role`). This regresses from the previous upsert behavior which refreshed `email` on conflict.
### Issue Context
The product allows changing auth email via settings, and email notifications query `public.users.email`.
### Fix Focus Areas
- src/app/baseball/actions/onboarding.ts[73-89]
### Suggested fix
Ensure `email` is refreshed for existing rows:
- If `existing.role === 'admin'`, keep role as admin but still allow email update (unless there’s a deliberate reason not to).
- For non-admin rows, update both `role` (when needed) and `email` (when it differs or unconditionally).
- Consider folding this into the same atomic/guarded update proposed in the admin-preservation fix.
ⓘ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/admin/_components/PanelBoundary.tsx (1)
78-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFocus is silently lost on remount — add a stable focus anchor.
key={attempt}fully unmounts/remountsPanelErrorBoundaryon every retry. The "Try again" button (and the whole<section>) that had keyboard focus at click time is destroyed, and nothing refocuses the recovered/still-erroring content. Keyboard and screen-reader users lose their place after every retry with no announcement of success/failure.Wrap the keyed subtree in a stable container and move focus to it once
attemptchanges.♿ Proposed fix to restore focus after remount
-import { Component, Suspense, useCallback, useState, useTransition, type ReactNode } from 'react'; +import { Component, Suspense, useCallback, useEffect, useRef, useState, useTransition, type ReactNode } from 'react';const [attempt, setAttempt] = useState(0); + const containerRef = useRef<HTMLDivElement>(null); + + useEffect(() => { + if (attempt > 0) containerRef.current?.focus(); + }, [attempt]); + return ( - <PanelErrorBoundary - key={attempt} - title={title} - onRetryReset={() => setAttempt((n) => n + 1)} - > - <Suspense fallback={skeleton ?? <SkeletonStat />}>{children}</Suspense> - </PanelErrorBoundary> + <div ref={containerRef} tabIndex={-1}> + <PanelErrorBoundary + key={attempt} + title={title} + onRetryReset={() => setAttempt((n) => n + 1)} + > + <Suspense fallback={skeleton ?? <SkeletonStat />}>{children}</Suspense> + </PanelErrorBoundary> + </div> );🤖 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 `@src/app/admin/_components/PanelBoundary.tsx` around lines 78 - 99, The retry remount in PanelBoundary is destroying the focused error UI without restoring focus. Keep the attempt-based remount for PanelErrorBoundary, but add a stable wrapper/focus anchor around the keyed subtree and, after onRetryReset increments attempt, move focus to that anchor so keyboard and screen-reader users stay oriented. Use the PanelBoundary and PanelErrorBoundary structure here to place the focus target outside the keyed remount and update focus on attempt changes.
🤖 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 `@src/app/admin/_components/PanelBoundary.tsx`:
- Around line 21-26: The retry path in PanelBoundary still depends on cached
admin fetches, so router.refresh() alone may replay stale upstream data. Update
the admin fetch flows in sentry-api and vercel-api to bypass cache on retry,
using cache: 'no-store' or a retry-specific revalidation/tag mechanism, and wire
that through the retry handling in PanelBoundary so the refresh forces a fresh
pull.
In `@src/app/baseball/actions/onboarding.ts`:
- Around line 486-489: The completeBaseballSignup flow is ignoring the result
from ensureUserRowPreservingAdmin, so a failed users-row read/write can still
allow the rest of the signup to continue. Update completeBaseballSignup to
capture the helper’s return value and mirror the coach path’s userError handling
by returning a typed failure immediately when the user row cannot be ensured.
Use the ensureUserRowPreservingAdmin call in onboarding.ts as the fix point and
keep the downstream baseball_players insert/coach delegation from running after
that failure.
- Line 88: The role change in the onboarding flow still has a TOCTOU gap in the
`admin.from('users').update(...)` path inside `onboardUser`; add the guard
directly to the `UPDATE` by keeping the `userId` filter and excluding `role =
'admin'` in the same write. Then handle the case where the guarded update
affects zero rows as a conflict/error so a late role change cannot demote the
last admin.
---
Outside diff comments:
In `@src/app/admin/_components/PanelBoundary.tsx`:
- Around line 78-99: The retry remount in PanelBoundary is destroying the
focused error UI without restoring focus. Keep the attempt-based remount for
PanelErrorBoundary, but add a stable wrapper/focus anchor around the keyed
subtree and, after onRetryReset increments attempt, move focus to that anchor so
keyboard and screen-reader users stay oriented. Use the PanelBoundary and
PanelErrorBoundary structure here to place the focus target outside the keyed
remount and update focus on attempt changes.
🪄 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: a80b0d62-d702-4b4e-b6ff-c2fab80b5e74
📒 Files selected for processing (4)
src/app/admin/_components/PanelBoundary.tsxsrc/app/admin/_components/PanelStates.tsxsrc/app/admin/_components/__tests__/panel-boundary.test.tsxsrc/app/baseball/actions/onboarding.ts
Comment on lines
+21
to
+26
| const retry = useCallback(() => { | ||
| startTransition(() => { | ||
| router.refresh(); | ||
| onReset(); | ||
| }); | ||
| }, [router, onReset]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the data-fetching code behind admin panels to confirm cache opt-out
rg -n "cache:\s*'no-store'|cacheTag|cacheLife|fetch\(" src/app/admin --type=ts --type=tsx -C2Repository: njrini99-code/helmv3
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map likely admin files and inspect the retry boundary plus data fetches.
git ls-files 'src/app/admin/**' | sed -n '1,200p'
printf '\n--- PanelBoundary ---\n'
sed -n '1,220p' src/app/admin/_components/PanelBoundary.tsx
printf '\n--- admin file cache/fetch scan ---\n'
rg -n "cache:\\s*'no-store'|cacheTag|cacheLife|revalidatePath|revalidateTag|fetch\\(" src/app/admin -g '*.ts' -g '*.tsx' -C 2Repository: njrini99-code/helmv3
Length of output: 7338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- admin root page ---'
sed -n '1,220p' src/app/admin/page.tsx
printf '\n%s\n' '--- admin errors page ---'
sed -n '1,220p' src/app/admin/errors/page.tsx
printf '\n%s\n' '--- admin health page ---'
sed -n '1,220p' src/app/admin/health/page.tsx
printf '\n%s\n' '--- admin deploys page ---'
sed -n '1,220p' src/app/admin/deploys/page.tsx
printf '\n%s\n' '--- admin panel-related fetch scan ---'
rg -n "cache:\\s*'no-store'|revalidateTag|revalidatePath|unstable_noStore|noStore\\(|fetch\\(" src/app/admin/_components src/app/admin -g '*.ts' -g '*.tsx' -C 2Repository: njrini99-code/helmv3
Length of output: 27930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- admin data helper cache scan ---'
rg -n "cache:\\s*'no-store'|cache:\\s*'force-cache'|unstable_noStore|noStore\\(|fetch\\(" src/lib/admin -g '*.ts' -g '*.tsx' -C 2
printf '\n%s\n' '--- helper file list ---'
git ls-files 'src/lib/admin/**' | sed -n '1,200p'Repository: njrini99-code/helmv3
Length of output: 4090
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sentry api ---'
sed -n '1,220p' src/lib/admin/sentry-api.ts
printf '\n%s\n' '--- vercel api ---'
sed -n '1,240p' src/lib/admin/vercel-api.ts
printf '\n%s\n' '--- admin data helpers with fetch ---'
for f in src/lib/admin/data/overview.ts src/lib/admin/data/triage.ts src/lib/admin/data/errors.ts src/lib/admin/data/feature-health.ts src/lib/admin/data/briefing.ts; do
printf '\n### %s ###\n' "$f"
sed -n '1,260p' "$f"
doneRepository: njrini99-code/helmv3
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PanelBoundary line numbers ---'
nl -ba src/app/admin/_components/PanelBoundary.tsx | sed -n '1,120p'
printf '\n%s\n' '--- sentry-api line numbers ---'
nl -ba src/lib/admin/sentry-api.ts | sed -n '1,140p'
printf '\n%s\n' '--- vercel-api line numbers ---'
nl -ba src/lib/admin/vercel-api.ts | sed -n '1,180p'Repository: njrini99-code/helmv3
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PanelBoundary line numbers ---'
cat -n src/app/admin/_components/PanelBoundary.tsx | sed -n '1,120p'
printf '\n%s\n' '--- sentry-api line numbers ---'
cat -n src/lib/admin/sentry-api.ts | sed -n '1,140p'
printf '\n%s\n' '--- vercel-api line numbers ---'
cat -n src/lib/admin/vercel-api.ts | sed -n '1,180p'Repository: njrini99-code/helmv3
Length of output: 15609
Bypass cached admin fetches on retry src/app/admin/_components/PanelBoundary.tsx:21-26 only calls router.refresh(), but the panel data sources still opt into Next revalidation (src/lib/admin/sentry-api.ts:92-96, src/lib/admin/vercel-api.ts:76-79, src/lib/admin/vercel-api.ts:123-125). That can replay the same upstream response for up to 60s/900s after a retry. Use cache: 'no-store' or a retry-specific revalidation/tag path if the button is meant to force a fresh pull.
🤖 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 `@src/app/admin/_components/PanelBoundary.tsx` around lines 21 - 26, The retry
path in PanelBoundary still depends on cached admin fetches, so router.refresh()
alone may replay stale upstream data. Update the admin fetch flows in sentry-api
and vercel-api to bypass cache on retry, using cache: 'no-store' or a
retry-specific revalidation/tag mechanism, and wire that through the retry
handling in PanelBoundary so the refresh forces a fresh pull.
Source: Path instructions
| } | ||
| if (existing.role === 'admin' || existing.role === role) return { error: null }; | ||
|
|
||
| const { error } = await admin.from('users').update({ role }).eq('id', userId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
supabase-js 2.107 update neq filter chaining
💡 Result:
In supabase-js, including version 2.107.0, the.neq filter is fully supported and designed to be chained with other filters [1][2]. There were no breaking changes or specific updates regarding.neq filter chaining in the 2.107.0 release [3][4]. The.neq method is a standard part of the PostgrestFilterBuilder API, which allows you to combine multiple filter conditions sequentially to build complex queries [5][2]. Example of chaining.neq with other filters: const { data, error } = await supabase.from('cities').select('name, country_id').gte('population', 1000).neq('name', 'The Shire') // Chaining.neq.lt('population', 10000); Key points regarding.neq: - It matches rows where the column is not equal to the specified value [1]. - It does not include rows where the column is NULL; for NULL values, use.is(column, null) instead [1]. - It is fully chainable with other filter methods like.select,.eq,.gt,.lt, etc. [5][2].
Citations:
- 1: https://supabase.com/docs/reference/javascript/neq
- 2: https://supabase.github.io/postgrest-js/v2/classes/PostgrestFilterBuilder.html
- 3: https://github.com/supabase/supabase-js/releases/tag/v2.107.0
- 4: https://github.com/supabase/supabase-js/blob/master/CHANGELOG.md
- 5: https://supabase.com/docs/reference/javascript/v1/using-filters
🏁 Script executed:
set -euo pipefail
printf '\n## file map\n'
git ls-files src/app/baseball/actions/onboarding.ts
printf '\n## outline\n'
ast-grep outline src/app/baseball/actions/onboarding.ts --view expanded || true
printf '\n## relevant slice\n'
sed -n '1,220p' src/app/baseball/actions/onboarding.ts | cat -n
printf '\n## search for admin demotion logic\n'
rg -n "only admin|demot|role|self-escalation|admin" src/app/baseball/actions/onboarding.ts src/app/baseball/actions -n || trueRepository: njrini99-code/helmv3
Length of output: 32738
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/app/baseball/actions/onboarding.ts')
print(p.exists(), p.stat().st_size if p.exists() else None)
PYRepository: njrini99-code/helmv3
Length of output: 169
🏁 Script executed:
set -euo pipefail
printf '\n## knowledge map\n'
npm run knowledge:map -- --files src/app/baseball/actions/onboarding.ts
printf '\n## knowledge context\n'
npm run knowledge:context -- --files src/app/baseball/actions/onboarding.ts --task "verify admin-demotion race and update filter semantics in onboarding action"Repository: njrini99-code/helmv3
Length of output: 1322
🏁 Script executed:
set -euo pipefail
printf '\n## search repo for related logic\n'
rg -n "self-escalation|only admin|last admin|demot|admin.*role|role.*admin" src supabase memory . -g '!**/node_modules/**' || true
printf '\n## package version for supabase-js\n'
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
console.log(pkg.dependencies?.['`@supabase/supabase-js`'] || pkg.devDependencies?.['`@supabase/supabase-js`'] || 'missing');
JSRepository: njrini99-code/helmv3
Length of output: 50377
🏁 Script executed:
set -euo pipefail
printf '\n## locate users role schema/type\n'
rg -n "users.*role|role.*users|type.*role|enum.*role|role:" src/lib/types supabase memory src -g '!**/node_modules/**' | head -n 200Repository: njrini99-code/helmv3
Length of output: 24549
Guard the role update itself. src/app/baseball/actions/onboarding.ts:73-88 still has a TOCTOU: if users.role flips to admin after the select, the later update can still demote the only admin. Keep the guard on the UPDATE (.eq('id', userId).neq('role', 'admin')) and treat a zero-row write as a 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 `@src/app/baseball/actions/onboarding.ts` at line 88, The role change in the
onboarding flow still has a TOCTOU gap in the `admin.from('users').update(...)`
path inside `onboardUser`; add the guard directly to the `UPDATE` by keeping the
`userId` filter and excluding `role = 'admin'` in the same write. Then handle
the case where the guarded update affects zero rows as a conflict/error so a
late role change cannot demote the last admin.
Comment on lines
+486
to
+489
| // Ensure user record exists (never demotes an existing admin) | ||
| await ensureUserRowPreservingAdmin( | ||
| admin, user.id, userEmail, data.role === 'coach' ? 'coach' : 'player', | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
completeBaseballSignup discards the helper's { error } — a failed user-row write silently proceeds.
At src/app/baseball/actions/onboarding.ts:486-489 the return value is dropped. If ensureUserRowPreservingAdmin fails (read or write error), execution continues and inserts into baseball_players (or delegates to the coach action) with no backing users row — an inconsistent account state. The coach path at Line 170-174 correctly checks userError; mirror that here. This also violates the action-file rule to surface a typed reason rather than swallowing errors.
🛠️ Proposed fix
- // Ensure user record exists (never demotes an existing admin)
- await ensureUserRowPreservingAdmin(
- admin, user.id, userEmail, data.role === 'coach' ? 'coach' : 'player',
- );
+ // Ensure user record exists (never demotes an existing admin)
+ const { error: userError } = await ensureUserRowPreservingAdmin(
+ admin, user.id, userEmail, data.role === 'coach' ? 'coach' : 'player',
+ );
+ if (userError) {
+ await logServerError(`[Onboarding] Failed to upsert user: ${describeDbError(userError)}`, { action: 'onboarding.completeBaseballSignup' });
+ return { success: false, error: 'Unable to set up your account. Please try again.' };
+ }📝 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.
Suggested change
| // Ensure user record exists (never demotes an existing admin) | |
| await ensureUserRowPreservingAdmin( | |
| admin, user.id, userEmail, data.role === 'coach' ? 'coach' : 'player', | |
| ); | |
| // Ensure user record exists (never demotes an existing admin) | |
| const { error: userError } = await ensureUserRowPreservingAdmin( | |
| admin, user.id, userEmail, data.role === 'coach' ? 'coach' : 'player', | |
| ); | |
| if (userError) { | |
| await logServerError(`[Onboarding] Failed to upsert user: ${describeDbError(userError)}`, { action: 'onboarding.completeBaseballSignup' }); | |
| return { success: false, error: 'Unable to set up your account. Please try again.' }; | |
| } |
🤖 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 `@src/app/baseball/actions/onboarding.ts` around lines 486 - 489, The
completeBaseballSignup flow is ignoring the result from
ensureUserRowPreservingAdmin, so a failed users-row read/write can still allow
the rest of the signup to continue. Update completeBaseballSignup to capture the
helper’s return value and mirror the coach path’s userError handling by
returning a typed failure immediately when the user row cannot be ensured. Use
the ensureUserRowPreservingAdmin call in onboarding.ts as the fix point and keep
the downstream baseball_players insert/coach delegation from running after that
failure.
Source: Path instructions
njrini99-code
pushed a commit
that referenced
this pull request
Jul 3, 2026
…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>
This was referenced Jul 3, 2026
njrini99-code
added a commit
that referenced
this pull request
Jul 3, 2026
…se + drift guard (#775) * docs(audits): read-only Supabase drift verification report (#651, #728, #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> * fix(admin): close root-cause admin-rollup 42501 gap + add Supabase drift 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> * fix(db): harden Supabase drift guard --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Cursor <cursoragent@cursor.com>
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 2, 2026
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.
Root cause (H1 confirmed, H2 ruled out)
Every
get_admin_*_rollupSECURITY-DEFINER function gates onusers.role='admin'. The Bridge app layer gates on theSUPER_ADMIN_USER_IDSenv allowlist. Those two agreed until baseball onboarding's users upsert ({ onConflict: 'id' }, service-role client, noignoreDuplicates) clobbered the allowlisted super-admin's row fromadmin→coach. After that,requireSuperAdmin()passed and every panel's RPC died with 42501. H2 (session not reaching the RPC) ruled out:fetchAdminRollupAuses the request-scoped server client, and the RPC succeeds with only asubclaim in a simulated call.Already done live (no deploy needed)
users.rolerestored toadminfor admin@helmsportslabs.com (04:18Z).get_admin_dashboard_rollup/get_admin_rounds_rollupreturn data; exactly onerole='admin'row and it IS the allowlist entry — the two admin models agree again.This PR (prevents recurrence + honest UX)
ensureUserRowPreservingAdmin()at both baseball onboarding users-row writes: insert-if-missing, player↔coach conversion allowed, an existingadminis NEVER overwritten. (Golf onboarding already usedignoreDuplicatesand could not demote.)PanelBoundary/PanelStale: the fallback claimed "showing last known data" while rendering none. Copy now says "temporarily unavailable" and ships a real retry (router.refresh()+ boundary remount).panel-boundary.test.tsx: asserts the honest copy, the retry path, and that the misleading claim never returns (7/7 green).Chosen model per sweep option 2:
users.role='admin'stays the DB source of truth, env allowlist stays the app-layer second factor, and onboarding can no longer un-sync them — only one super-admin exists, so anis_super_admin()table indirection adds surface without adding safety today.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fz1ajgmqcEp1yPEzjnXgY5