Helm Bridge — unified /admin command center + total feature instrumentation (single deploy)

[njrini99-code]

W15 — Total Error-Capture Coverage (PR B: total feature instrumentation)

Stacks on PR A #718 (feat/helm-bridge-command-center → main, LOW risk, foundation

• board). This PR is the invasive, at-scale part: wraps every GolfHelm + CoachHelm
  server action in withAdminObserved({sport, feature, …}) and centralizes RLS-denial
  capture in the shared pagination helper. Zero behavior change anywhere — every wrap is
  an Impl + thin delegator, bodies byte-identical, verified per-batch by diff.

Do not merge yet — owner merges PR A first, then this one (see NOT-FOR-AUTO-MERGE
note at the bottom).

Result: 424/424 non-CRM golf+coachhelm server-action exports wrapped

The count is not a manifest — coverage-contract.foundation.test.ts's global tripwire
(Task 16a) walks src/app/golf/actions/** live on every test run, drops the spec §1.3
CRM/non-action-boundary exclusions, and asserts every remaining export carries a valid
registry-matching FeatureKey. A future PR that lands an unwrapped action file in this
directory fails CI automatically.

Batch table (Tasks 5–14)

Batch	Task	Feature key(s)	Files	Exports	Commit
B0	5	admin_dashboard	admin-bi-data.ts, admin-data.ts, admin-people-data.ts, admin-system-data.ts, admin-tracer-data.ts, admin/rollup-c.ts, admin/actions/triage.ts	14	027e3c4ce
B1	6	round_tracking, stats_analytics, qualifiers, my_qualifiers	golf.ts (round/qualifier fns), round-drafts.ts, stats.ts, stats-data.ts, stats-intelligence.ts, stats-leak-maps.ts, shot-analytics.ts, team-sg-baseline.ts, v3/qualifying.ts	52	24f220064
B2	7	calendar_events, academics_classes, notifications	golf.ts (event/notification fns), attendance.ts, calendar-feeds.ts, calendar-sync.ts, recurring-events.ts, event-documents.ts, coach-notifications.ts, player-notifications.ts, push-notifications.ts	49	6af19e66d
B3	8	task_management, travel, documents	tasks.ts, task-templates.ts, task-reminders.ts, travel.ts, documents.ts	63	a479f7d7a
B4	9	messaging, announcements	message-attachments.ts, actions/messages.ts (10 golf exports only), announcements.ts, communication.ts, golf.ts (createAnnouncement)	23	2f5f286b7
B5	10	roster_management, team_info, join_team_flow, auth_onboarding, settings	roster.ts, teams.ts, team-switcher.ts, auth.ts, onboarding.ts, access-code.ts, demo-access.ts, demo-tracking.ts, v3/notification-prefs.ts, golf.ts (roster fns)	35	20a67d2a1
B6	11	course_library, recruiting_prospect_tracking, player_hub, coach_dashboard, my_game_profile, whats_new	course-library.ts, courses.ts, golf.ts (saved-course fns), recruiting.ts, recruit-documents.ts, dashboard-data.ts, command-palette.ts, player-profile-stats.ts, whats-new.ts	51	396cc446a
B7	12	coachhelm_ai_engine, alerts_system, patterns_dashboard	insights.ts (engine fns), insight-delivery.ts, player-fingerprint.ts, alerts.ts, pattern-management.ts	34	eb4395f52
B8	13	insights_management, intelligence_dashboard, coachhelm_analytics, coaching_intelligence_settings	insights.ts (7 lifecycle fns, via the insights-remainder commit) + insight-management.ts, insight-evidence.ts, intelligence-dashboard.ts, team-category-insights.ts, coachhelm-data.ts, causal-relationships.ts, coachhelm-analytics.ts, player-effectiveness.ts, coaching-philosophy.ts (32 files-only)	39 (7+32)	83261f54d (insights remainder) + 974f43675 (files)
B9	14	player_coachhelm_dashboard, round_review_ai, development_plans_coach, my_development, drills_practice_rx, coachhelm_v3_goals	insights.ts (3 overrides, via the insights-remainder commit) + player-feedback.ts, insight-celebration.ts, round-reviews.ts, round-review-system.ts, round-recap.ts, v3/llm.ts, development.ts, drills.ts, v3/practice-rx.ts, v3/team-practice-rx.ts, v3/goals.ts, v3/goal-progress.ts, v3/focus-area-progress.ts, v3/intent.ts (61 files-only)	64 (3+61)	83261f54d (insights remainder) + a498625fc (files)

14+52+49+63+23+35+51+34+39+64 = 424. insights.ts is 26/26 wrapped across B7 (16) + the
B8/B9 insights-remainder commit (10: 7 insights_management + generateRoundReview→
round_review_ai + getPlayerFocusAreas→my_development + getPlayerCoachHelmDashboard
→player_coachhelm_dashboard).

Task 15 — Centralize RLS-denial capture

• T15-CORE (8639262de): widened fetchAllRows/fetchAllRowsResult's makeQuery
  error type to {message: string; code?: string | null}, added an additive optional
  third arg rlsCtx?: RlsCaptureCtx, and call maybeCaptureRlsDenial(error, …) in both
  if (error) branches — fire-and-forget, return/throw byte-identical. 16 new tests.
• T15b — call-site threading (chore/helm-bridge-t15b-threading, commit 11be4c774,
  merged via 67a6598ab): threads rlsCtx {table, action, feature, sport:'golf'} through
  every fetchAllRows/fetchAllRowsResult call site across the 13 named golf-action
  files + 2 route handlers (calendar/feeds/[token]/route.ts,
  cron/event-reminders/route.ts). Adds maybeCaptureRlsDenial beside the existing
  ad-hoc 42501 branches in the 6 named files (event-documents.ts, recruit-documents.ts,
  insights.ts, teams.ts, golf.ts, round-reviews.ts); admin-data.ts correctly
  skipped (service-role client, no RLS to capture). Pure third-argument addition —
  zero behavior change.
• grep -rn "maybeCaptureRlsDenial" src/app/baseball src/lib/baseball src/app/lifting src/lib/lifting = 0 (verified independently twice: once in T15b, once in this FINALIZE unit).

Task 16 — Lock the invariant + verification sweep

• 16a (9658a042e): flipped coverage-contract.foundation.test.ts's global tripwire
  from it.todo to a live test. Discovers (does not hard-code) every 'use server'
  action file under src/app/golf/actions/** minus the spec §1.3 exclusion manifest,
  plus the 10 golf exports of actions/messages.ts + admin/actions/triage.ts, and
  asserts every export is wrapped with a valid registry-matching FeatureKey. A second
  test re-derives the 424 count live. Also replaced 3 stale self-test assertions (which
  assumed alerts.ts was still unwrapped — it was wrapped in B7) with a dedicated
  never-shipped fixture (fixtures/unwrapped-actions.fixture.ts).
• FINALIZE (this PR's tip, commit 789389be3, merge 67a6598ab): merged T15b into
  the instrumentation branch and ran the one full-suite gate for the whole unit (below).
  Task 16 step 3 (manual forced-failure dev-environment verification — trigger one bad
  table-name failure, confirm a single tagged admin_events row + fingerprint collapse
  at 50×, confirm enterDemo/NEXT_REDIRECT writes nothing) requires a dev server and
  was not run in this CLI-only unit (machine rule: no dev servers/browsers) — left
  for the owner or a follow-up unit with dev-server access.

Full gate results (this PR's final state, one run)

npx tsc --noEmit         → exit 0, 0 errors
npm run lint              → exit 0, 0 errors (2275 pre-existing baseline warnings, 0 new)
npm test (vitest --project unit run, NODE_OPTIONS=--max-old-space-size=8192)
  → 3992 passed / 6 failed / 39 skipped   (4037 total, 376 test files: 372 passed, 3 failed, 1 skipped)

coverage-contract.* (13 batch files + foundation) + feature-registry.test.ts: all
green, including the flipped tripwire (every non-CRM golf action export is wrapped

• total wrapped-and-valid action count ... is exactly 424). fetch-all-rows.test.ts:
  13/13 green including every rlsCtx capture assertion.

Excused pre-existing failures (6/6, proof attached)

Proof method: git diff --stat 880994c16 HEAD -- <path> (880994c16 = the tip this
unit started from, before T15b/T16a/FINALIZE) produced zero output for every file
below — i.e. the implicated files are byte-identical between the pre-unit base tip and
this PR's HEAD, so today's failure is exactly yesterday's failure.

Test file	Failing tests	Cause	Proof
src/lib/baseball/__tests__/program-type-nav-variants.test.ts	3 (coach nav ORDER differs between College and Showcase, absent programType falls back to declaration order, re-ordering never adds or drops entries)	Pre-existing baseball nav-registry/nav-manifest entry-set drift (events/organization/teams), unrelated to golf/admin instrumentation. Out of scope per hard rule "never touch baseball/lifting/crm."	git diff --stat 880994c16 HEAD -- src/lib/baseball/ = empty. Re-ran in isolation — same 3/16 failures, deterministic.
src/test/golf/actions/insight-celebration.test.ts	1 (stamps metadata.celebration_shown_at on first call)	Known Next 16 Invariant: static generation store missing in revalidatePath — revalidatePath() invoked outside request scope inside vitest. Documented in every prior W15 batch log entry (B6, B8/B9, T15-CORE, T16a).	git diff --stat 880994c16 HEAD -- src/app/golf/actions/insight-celebration.ts src/lib/admin/observed-action.ts = empty.
src/app/golf/actions/__tests__/round-recap.test.ts	2 (9-hole vs 18-hole average comparison tests)	Same Next 16 revalidatePath-outside-request-scope invariant, in round-recap.ts:109.	git diff --stat 880994c16 HEAD -- src/app/golf/actions/round-recap.ts src/lib/admin/observed-action.ts = empty.

The third documented KNOWN class (jsdom node_modules/node_modules/react duplicate-module
errors) did not trigger this run — nothing to excuse.

Hard-rule compliance

• Zero touches to crm-*.ts, resend-activity.ts, src/app/baseball/**,
  src/app/lifting/**, src/lib/baseball/**, src/lib/lifting/**, the self-referential
  log sinks, inngest/route.ts, or the 19 already-instrumented /api/cron/** handlers.
• Every wrap is Impl + thin delegator — no export const x = withAdminObserved(...)
  (which breaks Next's build in golf's 'use server' files).
• Wrapper logging is fire-and-forget; NEXT_REDIRECT/NEXT_NOT_FOUND are still control
  flow, never logged; {success:false} validation envelopes are still not errors.
• No migrations in this PR.

⚠ NOT FOR AUTO-MERGE

This branch is stacked on feat/helm-bridge-command-center (PR A, #718), not main.
Owner merges PR A first, then this PR. Nothing in this unit was merged into main or
batch/baseball-fixes.

🤖 Generated with Claude Code

[njrini99-code]

🤖 Mission Control — PR summary

What it changes: Consolidates the two Helm Bridge PRs into a single-deploy change. Ships the unified /admin command center (console + Feature Health board — formerly PR A #718, now closed unmerged and folded in here) and the at-scale instrumentation pass: wraps every non-CRM GolfHelm + CoachHelm server action in withAdminObserved({sport, feature, …}) and centralizes RLS-denial capture in the shared pagination helper. Claims 424/424 golf+coachhelm action exports wrapped, enforced by a live coverage tripwire (coverage-contract.foundation.test.ts). +30,350 / −940 across 256 files. Base: main.

Risk / areas: HIGH surface area — ~74 files under src/app (essentially the entire golf/coachhelm server-action layer) plus the admin console and .circleci/config.yml. Author asserts every wrap is an Impl + thin delegator with byte-identical bodies (zero behavior change), verified per-batch by diff. The blast radius is the whole server-action layer, so the "zero behavior change" claim is the crux of the review.

⚠️ CI is RED — do not merge in this state:

• ❌ Unit tests
• ❌ Business contracts
• ❌ Supabase lint + RLS tests
• ❌ Playwright (chromium)
• ❌ semgrep (custom rules)
• ❌ Review Gate / all
• ⏳ Pending: CodeRabbit, BaseballHelm seeded smoke, Course picker screenshots
• ✅ Passing: TypeScript, Next build, ESLint, Lint ratchet, Schema invariants, Route Hygiene P0/P1, gitleaks, sqlfluff, ast-grep, actionlint, etc.

What reviewers should watch:

1. The 4 failing test suites — confirm they're real regressions vs. the pre-existing baseball nav-variant / revalidatePath-outside-request-scope failures the author references elsewhere.
2. The semgrep custom-rule failure — could be a real hard-gate trip (service-role-in-client / RLS-missing / server-action-without-auth-check rule), not just noise.
3. Spot-check a sample of Impl+delegator wraps for genuinely byte-identical bodies, given the mechanical AST retrofit across 256 files.
4. That PR A Helm Bridge — unified /admin command center (PR A: console + Feature Health) #718's console work is fully represented here, since Helm Bridge — unified /admin command center (PR A: console + Feature Health) #718 was closed without merging.

Automated summary · not a review · CI status as of 2026-07-02 22:07 UTC.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (3) 📜 Skill insights (2)

Context used

✅ Compliance rules (platform): 93 rules

✅ Skills: modern-saas-ui, pproenca-dot-skills-framer-motion

1. admin_allowlist has no policy 📘 Rule violation ⛨ Security

Description

The new public.admin_allowlist table enables (and forces) RLS but defines no CREATE POLICY in
the same migration. This violates the requirement that every newly created table migration must
include at least one RLS policy, risking audit failure and inconsistent access behavior
expectations.

Code

supabase/migrations/20260701110000_admin_allowlist_is_super_admin.sql[R6-14]

+CREATE TABLE IF NOT EXISTS public.admin_allowlist (
+  user_id uuid PRIMARY KEY REFERENCES auth.users(id) ON DELETE CASCADE,
+  email text NOT NULL,
+  note text,
+  created_at timestamptz NOT NULL DEFAULT now()
+);
+
+ALTER TABLE public.admin_allowlist ENABLE ROW LEVEL SECURITY;
+ALTER TABLE public.admin_allowlist FORCE ROW LEVEL SECURITY;

Relevance

⭐⭐⭐ High

Supabase migration governance/security fixes are accepted (e.g., RLS/grants hardening and migration
standards).

PR-#517
PR-#296

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1519303 requires at least one RLS policy per new table migration. The migration
creates public.admin_allowlist and enables/forces RLS, but contains no CREATE POLICY statements.

Rule 1519303: Require RLS enablement and at least one policy per new table migration
supabase/migrations/20260701110000_admin_allowlist_is_super_admin.sql[6-14]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new table (`public.admin_allowlist`) is created with RLS enabled/forced but no RLS policies are defined in the same migration.

## Issue Context
Compliance requires every new table migration to both enable RLS and define at least one policy.

## Fix Focus Areas
- supabase/migrations/20260701110000_admin_allowlist_is_super_admin.sql[6-14]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Anonymous log spam risk 🐞 Bug ⛨ Security

Description

POST /api/log-error no longer requires an authenticated user and performs privileged inserts into
error_logs/admin_events via the service-role admin client, enabling unauthenticated callers to
generate DB write load and telemetry noise. The only mitigation is a per-instance in-memory rate
limit, which does not prevent distributed flooding across IPs/instances.

Code

src/app/api/log-error/route.ts[R15-23]

    const supabase = await createClient();
    const { data: { user } } = await supabase.auth.getUser();
-
-    if (!user) {
-      return NextResponse.json({ success: false }, { status: 401 });
-    }
+    // Was: 401 for unauthenticated users — which blinded us to login/signup
+    // flow client errors (they reached Sentry but never error_logs).
+    // Anonymous writes are accepted, flagged, and severity-capped.
+    const isAnonymous = !user;

    const errorReport = await request.json();
    const adminClient = createAdminClient();

Relevance

⭐⭐ Medium

Repo has precedent for public/admin-client writes with suppressions; no clear decision pattern for
anon log inserts.

PR-#304
PR-#290

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The route explicitly treats missing user as acceptable (isAnonymous) and still inserts into
error_logs/admin_events using createAdminClient() (service-role). The only protection is
withRateLimit backed by an in-memory Map, which is not effective across horizontally scaled
instances or distributed callers.

src/app/api/log-error/route.ts[8-99]
src/lib/supabase/admin.ts[4-21]
src/lib/rate-limit.ts[1-21]
src/lib/rate-limit.ts[110-128]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`POST /api/log-error` now accepts unauthenticated requests and writes into `error_logs` and `admin_events` using the service-role admin client. This expands the trust boundary and allows unauthenticated log spam / DB write amplification; the current limiter is in-memory per instance.

## Issue Context
The route intentionally allows anonymous reporting for pre-auth flows, but it still needs stronger abuse controls (request size, stricter limits, and/or narrower acceptance rules) because it writes through `SUPABASE_SERVICE_ROLE_KEY`.

## Fix Focus Areas
- src/app/api/log-error/route.ts[14-109]
- src/lib/rate-limit.ts[1-21]
- src/lib/rate-limit.ts[110-128]
- src/lib/supabase/admin.ts[4-21]

## Suggested changes
- Add a request body size guard *before* JSON parsing (e.g., read `await request.text()`, reject with 413 if > N bytes, then `JSON.parse`).
- Tighten anonymous acceptance rules:
 - Use a much stricter rate limit for anonymous reports (separate bucket from `API_WRITE`).
 - Consider accepting anonymous reports only when `new URL(request.url).pathname`/`referer` indicates known pre-auth pages (login/signup) and otherwise return 401.
 - Optionally avoid inserting anonymous reports into `admin_events` (keep them in `error_logs` only) to reduce operational noise.
- If you need stronger guarantees, move rate limiting to a shared store (Upstash/Redis) so limits apply across instances.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Static LazyMotion features import 📜 Skill insight ➹ Performance

Description

AdminMotionProvider passes domAnimation via a static import to LazyMotion instead of using a
dynamic import function. This violates the bundle-optimization requirement and can increase client
bundle cost.

Code

src/app/admin/_motion-provider.tsx[R3-12]

+import { LazyMotion, domAnimation } from 'framer-motion';
+import type { ReactNode } from 'react';
+
+/**
+ * LazyMotion(domAnimation) at the /admin route root. Without it every
+ * `<m.*>` renders as static DOM and animated numbers freeze at 0 — this
+ * bit the golf-admin Tracer KPI tiles before (see golf/admin/layout.tsx).
+ */
+export function AdminMotionProvider({ children }: { children: ReactNode }) {
+  return <LazyMotion features={domAnimation}>{children}</LazyMotion>;

Relevance

⭐⭐ Medium

No historical evidence found about enforcing LazyMotion dynamic features imports in this repo.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1519405 requires a dynamic import function for LazyMotion's features prop, but
the code imports domAnimation statically and passes it directly to LazyMotion.

src/app/admin/_motion-provider.tsx[3-12]
Skill: pproenca-dot-skills-framer-motion

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LazyMotion` features are provided via a static import (`domAnimation`) instead of a dynamic import loader function.

## Issue Context
The compliance rule requires `features` to be a function returning a dynamic import for better bundle optimization.

## Fix Focus Areas
- src/app/admin/_motion-provider.tsx[3-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. LazyMotion missing strict 📜 Skill insight ☼ Reliability

Description

AdminMotionProvider renders LazyMotion without the strict prop. This violates the requirement
to enable strict mode to prevent accidental motion imports from silently pulling the full Framer
Motion bundle.

Code

src/app/admin/_motion-provider.tsx[R11-12]

+export function AdminMotionProvider({ children }: { children: ReactNode }) {
+  return <LazyMotion features={domAnimation}>{children}</LazyMotion>;

Relevance

⭐⭐ Medium

No repo history found enforcing LazyMotion strict prop; no similar accepted/rejected examples.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1519412 requires LazyMotion to include strict, but the provider returns
<LazyMotion features={domAnimation}> without it.

src/app/admin/_motion-provider.tsx[11-12]
Skill: pproenca-dot-skills-framer-motion

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LazyMotion` is used without the `strict` prop.

## Issue Context
Strict mode helps catch accidental `motion` imports that can bloat bundles.

## Fix Focus Areas
- src/app/admin/_motion-provider.tsx[11-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Job logging blocks cron 🐞 Bug ☼ Reliability

Description

recordJobRun awaits an insert into background_job_logs on both success and failure paths,
placing DB I/O on the critical path of cron handlers. If Supabase is slow/unavailable this delays
cron responses despite the intended fire-and-forget contract.

Code

src/lib/admin/job-log.ts[R20-58]

+export async function recordJobRun<T>(jobType: string, fn: () => Promise<T>): Promise<T> {
+  const startedAt = new Date();
+  try {
+    const result = await fn();
+    await writeRow(jobType, 'completed', startedAt, null);
+    return result;
+  } catch (err) {
+    const message = err instanceof Error ? err.message : String(err);
+    await writeRow(jobType, 'failed', startedAt, message.slice(0, 2000));
+    try {
+      void logServerEvent(
+        `Cron failed: ${jobType}`,
+        { action: `cron.${jobType}`, source: 'cron', errorDetails: message.slice(0, 2000) },
+        'error',
+      ).catch(() => {});
+    } catch {
+      /* never mask the real failure */
+    }
+    throw err;
+  }
+}
+
+async function writeRow(
+  jobType: string,
+  status: 'completed' | 'failed',
+  startedAt: Date,
+  errorMessage: string | null,
+): Promise<void> {
+  try {
+    const completedAt = new Date();
+    const admin = createAdminClient();
+    await admin.from('background_job_logs').insert({
+      job_type: jobType,
+      status,
+      duration_ms: completedAt.getTime() - startedAt.getTime(),
+      error_message: errorMessage,
+      started_at: startedAt.toISOString(),
+      completed_at: completedAt.toISOString(),
+    });

Relevance

⭐⭐ Medium

No direct precedent on avoiding awaited DB inserts in cron critical path; cron reliability
suggestions often rejected.

PR-#45
PR-#332

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

recordJobRun awaits writeRow, and writeRow awaits a Supabase insert. Cron routes call
recordJobRun directly, so the DB insert latency is part of the cron handler’s response time.

src/lib/admin/job-log.ts[20-58]
src/app/api/cron/v3/weekly-coach-email/route.ts[38-47]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`recordJobRun` claims fire-and-forget semantics but performs `await writeRow(...)`, and `writeRow` performs an awaited DB insert. Errors are swallowed, but latency is still added to every cron request.

## Issue Context
Cron handlers now wrap their work in `recordJobRun(...)`. Keeping the logging call awaited increases the chance that a slow DB extends job runtime unnecessarily.

## Fix Focus Areas
- src/lib/admin/job-log.ts[20-58]
- src/app/api/cron/v3/weekly-coach-email/route.ts[38-47]

## Suggested changes
- Remove the awaited DB write from the critical path:
 - On success: `void writeRow(jobType, 'completed', startedAt, null);`
 - On failure: `void writeRow(jobType, 'failed', startedAt, ...);` before rethrow.
- Optionally bound the logging latency even when fire-and-forget:
 - Use an internal timeout/abort (if supported by your Supabase client) or wrap the insert in a `Promise.race` with a short timer.
- Keep the existing `try/catch` swallowing so logging failure never affects the cron outcome.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. buildDigestEmail hardcodes #16A34A 📘 Rule violation ⚙ Maintainability

Description

The digest email HTML uses the raw hex color #16A34A inline instead of the designated
primary-600 token. This violates the design-token requirement and makes brand color updates
harder.

Code

src/lib/admin/digest/build-digest.ts[R65-68]

+  const html = `<!doctype html><html><body style="font-family:ui-monospace,Menlo,monospace;background:#faf8f2;color:#1c1917;padding:24px">
+  <h2 style="margin:0 0 4px">${esc(subject)}</h2>
+  <p style="color:${redCount > 0 ? '#DC2626' : '#16A34A'};font-weight:600">
+    ${redCount > 0 ? `${redCount} item${redCount === 1 ? '' : 's'} need attention` : 'All systems nominal'}

Relevance

⭐ Low

Design-token/inline-hex cleanup suggestions often rejected or left unaddressed historically.

PR-#274
PR-#304

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1519245 forbids direct usage of #16A34A in modified UI/styling code; the digest
email HTML contains '#16A34A' inline in the style attribute.

Rule 1519245: Use the designated brand primary color token instead of hard-coded hex values
src/lib/admin/digest/build-digest.ts[65-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The HTML template uses a hard-coded `#16A34A` value.

## Issue Context
Brand colors must be expressed via the designated token (`primary-600`) rather than raw hex literals.

## Fix Focus Areas
- src/lib/admin/digest/build-digest.ts[65-68]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Digest types not in @/lib/types 📘 Rule violation ⚙ Maintainability

Description

DigestData and DigestEmail are defined in-module and imported as types from non-@/lib/types
paths across multiple modules. This violates the shared-type centralization and type-import path
requirements, increasing duplication risk.

Code

src/lib/admin/digest/build-digest.ts[R1-18]

+export interface DigestData {
+  generatedAt: string;
+  errors24h: {
+    total: number;
+    critical: number;
+    topIncidents: Array<{ title: string; occurrences: number; affectedUsers: number }>;
+  };
+  sentry: { unresolved: number | null; regressed: number | null };
+  signups24h: Array<{ email: string; role: string }>;
+  activity24h: { golfRounds: number; baseballGames: number; liftSessions: number };
+  reds: string[];
+}
+
+export interface DigestEmail {
+  subject: string;
+  html: string;
+  text: string;
+}

Relevance

⭐ Low

Type import-path centralization suggestions previously rejected (canonical @/lib/types barrel
enforcement not adopted).

PR-#290

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1519222 requires shared domain types used across modules to be moved to and
imported from @/lib/types, and PR Compliance ID 1519311 requires type-only imports to come from
@/lib/types. The digest types are declared in build-digest.ts and then imported as types from
other non-@/lib/types modules.

Rule 1519222: Import shared type definitions from '@/lib/types' instead of defining them inline
Rule 1519311: Type imports must use '@/lib/types' and disallow deprecated paths
src/lib/admin/digest/build-digest.ts[1-18]
src/lib/admin/digest/transport.ts[1-3]
src/app/api/cron/admin-digest/route.ts[1-8]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shared digest types (`DigestData`, `DigestEmail`) are declared in `build-digest.ts` and imported as types from other module paths instead of being centralized in `@/lib/types`.

## Issue Context
These types are used across multiple modules (digest builder, transport, and cron route), so they should be defined once in `@/lib/types` and imported from there.

## Fix Focus Areas
- src/lib/admin/digest/build-digest.ts[1-18]
- src/lib/admin/digest/transport.ts[1-3]
- src/app/api/cron/admin-digest/route.ts[1-8]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Dropped DB error details 🐞 Bug ◔ Observability

Description

rateInsightAsPlayer now throws generic errors on Supabase failures after removing logServerError
calls that previously captured the underlying PostgREST error details. Because withAdminObserved
logs only the thrown error, the original DB error message/code needed to debug RLS/grant issues is
lost.

Code

src/app/golf/actions/player-feedback.ts[R118-123]

    .maybeSingle();

  if (playerErr) {
-    await logServerError(
-      `rateInsightAsPlayer.player lookup failed: ${playerErr.message ?? 'unknown'}`,
-      {
-        action: 'rateInsightAsPlayer.player',
-        featureArea: 'player_feedback',
-        extra: { userId: user.id },
-      }
-    );
+    // W15: inline logServerError removed here — withAdminObserved now
+    // captures this throw at the export boundary (no more double-log).
    throw new Error('Player lookup failed');

Relevance

⭐ Low

Similar “add more error handling/details” suggestions frequently rejected in app code reviews.

PR-#564
PR-#274

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The error branches now discard playerErr/insightErr/upsertErr details and throw only generic
messages. The wrapper logs the caught exception object (the generic Error) and does not
automatically attach the original Supabase error payload, so the DB error context is no longer
recorded.

src/app/golf/actions/player-feedback.ts[112-173]
src/lib/admin/observed-action.ts[30-60]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In `rateInsightAsPlayer`, the code catches `playerErr` / `insightErr` / `upsertErr` but now throws generic `Error(...)` messages without preserving the original Supabase error payload. Since the export is wrapped in `withAdminObserved`, only the generic error is captured, reducing diagnostic fidelity.

## Issue Context
The PR removed inline `logServerError` to avoid double logging, but this also removed the only place where the original DB error message/code was recorded.

## Fix Focus Areas
- src/app/golf/actions/player-feedback.ts[112-173]
- src/lib/admin/observed-action.ts[30-60]

## Suggested changes
- Preserve root-cause details while still relying on `withAdminObserved`:
 - Throw with a `cause` that carries the original DB error (wrap non-Error payloads):
   - `throw new Error('Player lookup failed', { cause: new Error(playerErr.message) })`
   - same pattern for `insightErr` and `upsertErr`
 - If error codes exist (e.g., `code`), include them in the `cause` message or in a structured `Error` subclass.
- Alternatively, reintroduce `logServerError(...)` but set `{ skipSentry: true }` (so it still records to DB/admin feed without duplicating Sentry issues), and keep the thrown error generic for the client.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

1. admin_allowlist has no policy 📘 Rule violation ⛨ Security

The new public.admin_allowlist table enables (and forces) RLS but defines no CREATE POLICY in
the same migration. This violates the requirement that every newly created table migration must
include at least one RLS policy, risking audit failure and inconsistent access behavior
expectations.

Agent Prompt

## Issue description
A new table (`public.admin_allowlist`) is created with RLS enabled/forced but no RLS policies are defined in the same migration.

## Issue Context
Compliance requires every new table migration to both enable RLS and define at least one policy.

## Fix Focus Areas
- supabase/migrations/20260701110000_admin_allowlist_is_super_admin.sql[6-14]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

2. Static lazymotion features import 📜 Skill insight ➹ Performance

AdminMotionProvider passes domAnimation via a static import to LazyMotion instead of using a
dynamic import function. This violates the bundle-optimization requirement and can increase client
bundle cost.

Agent Prompt

## Issue description
`LazyMotion` features are provided via a static import (`domAnimation`) instead of a dynamic import loader function.

## Issue Context
The compliance rule requires `features` to be a function returning a dynamic import for better bundle optimization.

## Fix Focus Areas
- src/app/admin/_motion-provider.tsx[3-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

3. lazymotion missing strict 📜 Skill insight ☼ Reliability

AdminMotionProvider renders LazyMotion without the strict prop. This violates the requirement
to enable strict mode to prevent accidental motion imports from silently pulling the full Framer
Motion bundle.

Agent Prompt

## Issue description
`LazyMotion` is used without the `strict` prop.

## Issue Context
Strict mode helps catch accidental `motion` imports that can bloat bundles.

## Fix Focus Areas
- src/app/admin/_motion-provider.tsx[11-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

6. Anonymous log spam risk 🐞 Bug ⛨ Security

POST /api/log-error no longer requires an authenticated user and performs privileged inserts into
error_logs/admin_events via the service-role admin client, enabling unauthenticated callers to
generate DB write load and telemetry noise. The only mitigation is a per-instance in-memory rate
limit, which does not prevent distributed flooding across IPs/instances.

Agent Prompt

## Issue description
`POST /api/log-error` now accepts unauthenticated requests and writes into `error_logs` and `admin_events` using the service-role admin client. This expands the trust boundary and allows unauthenticated log spam / DB write amplification; the current limiter is in-memory per instance.

## Issue Context
The route intentionally allows anonymous reporting for pre-auth flows, but it still needs stronger abuse controls (request size, stricter limits, and/or narrower acceptance rules) because it writes through `SUPABASE_SERVICE_ROLE_KEY`.

## Fix Focus Areas
- src/app/api/log-error/route.ts[14-109]
- src/lib/rate-limit.ts[1-21]
- src/lib/rate-limit.ts[110-128]
- src/lib/supabase/admin.ts[4-21]

## Suggested changes
- Add a request body size guard *before* JSON parsing (e.g., read `await request.text()`, reject with 413 if > N bytes, then `JSON.parse`).
- Tighten anonymous acceptance rules:
  - Use a much stricter rate limit for anonymous reports (separate bucket from `API_WRITE`).
  - Consider accepting anonymous reports only when `new URL(request.url).pathname`/`referer` indicates known pre-auth pages (login/signup) and otherwise return 401.
  - Optionally avoid inserting anonymous reports into `admin_events` (keep them in `error_logs` only) to reduce operational noise.
- If you need stronger guarantees, move rate limiting to a shared store (Upstash/Redis) so limits apply across instances.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

8. Job logging blocks cron 🐞 Bug ☼ Reliability

recordJobRun awaits an insert into background_job_logs on both success and failure paths,
placing DB I/O on the critical path of cron handlers. If Supabase is slow/unavailable this delays
cron responses despite the intended fire-and-forget contract.

Agent Prompt

## Issue description
`recordJobRun` claims fire-and-forget semantics but performs `await writeRow(...)`, and `writeRow` performs an awaited DB insert. Errors are swallowed, but latency is still added to every cron request.

## Issue Context
Cron handlers now wrap their work in `recordJobRun(...)`. Keeping the logging call awaited increases the chance that a slow DB extends job runtime unnecessarily.

## Fix Focus Areas
- src/lib/admin/job-log.ts[20-58]
- src/app/api/cron/v3/weekly-coach-email/route.ts[38-47]

## Suggested changes
- Remove the awaited DB write from the critical path:
  - On success: `void writeRow(jobType, 'completed', startedAt, null);`
  - On failure: `void writeRow(jobType, 'failed', startedAt, ...);` before rethrow.
- Optionally bound the logging latency even when fire-and-forget:
  - Use an internal timeout/abort (if supported by your Supabase client) or wrap the insert in a `Promise.race` with a short timer.
- Keep the existing `try/catch` swallowing so logging failure never affects the cron outcome.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools