diff --git a/.github/branch-protection.md b/.github/branch-protection.md index 402433fb9..fa939bbe1 100644 --- a/.github/branch-protection.md +++ b/.github/branch-protection.md @@ -20,6 +20,9 @@ so a job split/rename does not silently break protection. - `Next build` - `Route Hygiene P0/P1` - `Supabase lint + RLS tests` + - `BaseballHelm authenticated smoke` (coach + player, #372) — skips (does + not fail) on fork/Dependabot PRs, which receive no repo secrets from + GitHub by design - `Review Gate / all` — hard aggregate for ast-grep, semgrep, gitleaks, actionlint, yamllint, shellcheck, markdownlint, ruff+pylint, sqlfluff, and hadolint. @@ -48,6 +51,20 @@ Advisory checks: `CI / all` aggregate once the baseball pgTAP RLS suite went green on `main` (#517, supersedes #423). RLS regressions now block merge. +`BaseballHelm authenticated smoke` (the `baseball-auth-smoke` job in +`ci.yml`) was promoted the same way (#372): the coach/player smoke suite +(`e2e/baseball-smoke.spec.ts` + `e2e/baseball-onboarding-smoke.spec.ts`) and +its fail-loud auth setup already existed and ran on every `main` push via +`playwright.yml`'s `e2e` job, but only post-merge — a real authenticated +regression could land on `main` before this ever ran. `ci.yml` now runs the +same specs (steps copied, not moved) as a required PR gate. It skips rather +than fails on fork/Dependabot PRs (no repo secrets available to them); +same-repo, non-Dependabot pushes and PRs must have the required secrets +configured or the job fails loudly. Note the added cost: a second full +`npm run build` + Playwright-chromium install on every same-repo, +non-Dependabot PR, on top of the existing `Next build` / `Smoke checks` +builds. + `Greptile Review` is intentionally advisory, not a required check: Greptile's `.greptile/config.json` skips `dependabot`-titled PRs, so it never posts a passing `Greptile Review` on them — making it a required context would leave diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5d2231ff7..01bc192ed 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -312,6 +312,126 @@ jobs: - name: Check for new runtime import cycles run: npm run check:cycles + baseball-auth-smoke: + name: BaseballHelm authenticated smoke + # #372: promotes the already-built, already-mandatory baseball-smoke + + # onboarding-smoke specs (e2e/baseball-smoke.spec.ts, + # e2e/baseball-onboarding-smoke.spec.ts) from push/workflow_dispatch-only + # (.github/workflows/playwright.yml's `e2e` job) into a required PR gate — + # the same mechanism that promoted Supabase RLS tests into `all` (#517) + # and the import-cycle ratchet (#808). Steps below are copied verbatim + # from playwright.yml's readiness-check + fail-loud pattern, not moved: + # the push/manual job keeps running the full mandatory suite (plus the + # mobile-viewport regression) unchanged. + # + # Skips (does not fail) on fork/Dependabot pull_request runs, which + # receive no repo secrets from GitHub by design — `all`'s aggregate step + # only treats 'failure'/'cancelled' as blocking, so a skipped job here + # never blocks an external contribution that structurally cannot supply + # these secrets. Same-repo, non-Dependabot pushes and PRs still hard-fail + # on missing secrets, same as the push-triggered version always has. + # Dependabot PRs are opened against this repo (not a fork), so the + # same-repo check alone does not exclude them — `github.actor` is + # checked explicitly for that reason. + # + # COST NOTE: this doubles Playwright-browser-install + a full `npm run + # build` on every PR from this repo, on top of the existing next-build / + # Smoke-checks builds — a real, ongoing CI-minutes tradeoff the repo had + # previously avoided by scoping full E2E to push-only. Revisit if PR + # wall-clock time becomes a problem. + if: >- + github.event_name == 'push' || + (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && github.actor != 'dependabot[bot]') + runs-on: ubuntu-latest + timeout-minutes: 30 + env: + NEXT_PUBLIC_SUPABASE_URL: ${{ secrets.NEXT_PUBLIC_SUPABASE_URL || 'https://dummy-ci-build.supabase.co' }} + NEXT_PUBLIC_SUPABASE_ANON_KEY: ${{ secrets.NEXT_PUBLIC_SUPABASE_ANON_KEY || 'dummy-anon-key-ci-build-only' }} + SUPABASE_SERVICE_ROLE_KEY: ${{ secrets.SUPABASE_SERVICE_ROLE_KEY }} + E2E_BASEBALL_COACH_EMAIL: ${{ secrets.E2E_BASEBALL_COACH_EMAIL }} + E2E_BASEBALL_COACH_PASSWORD: ${{ secrets.E2E_BASEBALL_COACH_PASSWORD }} + E2E_BASEBALL_PLAYER_EMAIL: ${{ secrets.E2E_BASEBALL_PLAYER_EMAIL }} + E2E_BASEBALL_PLAYER_PASSWORD: ${{ secrets.E2E_BASEBALL_PLAYER_PASSWORD }} + NODE_OPTIONS: --max-old-space-size=8192 + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + persist-credentials: false + + - name: Setup Node + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e + with: + node-version: 22 + cache: npm + + - name: Install dependencies + run: npm ci + + - name: Compute BaseballHelm auth readiness + id: baseball-auth-ready + run: | + missing=() + [ -z "$SUPABASE_SERVICE_ROLE_KEY" ] && missing+=("SUPABASE_SERVICE_ROLE_KEY") + [ -z "$E2E_BASEBALL_COACH_EMAIL" ] && missing+=("E2E_BASEBALL_COACH_EMAIL") + [ -z "$E2E_BASEBALL_COACH_PASSWORD" ] && missing+=("E2E_BASEBALL_COACH_PASSWORD") + [ -z "$E2E_BASEBALL_PLAYER_EMAIL" ] && missing+=("E2E_BASEBALL_PLAYER_EMAIL") + [ -z "$E2E_BASEBALL_PLAYER_PASSWORD" ] && missing+=("E2E_BASEBALL_PLAYER_PASSWORD") + + if [ ${#missing[@]} -gt 0 ]; then + echo "ready=false" >> "$GITHUB_OUTPUT" + printf 'missing=%s\n' "${missing[*]}" >> "$GITHUB_OUTPUT" + echo "BaseballHelm auth secrets unavailable: ${missing[*]}" + else + echo "ready=true" >> "$GITHUB_OUTPUT" + echo "missing=" >> "$GITHUB_OUTPUT" + echo "BaseballHelm auth secrets available." + fi + + - name: Fail missing BaseballHelm secrets + if: steps.baseball-auth-ready.outputs.ready != 'true' + env: + MISSING_BASEBALL_SECRETS: ${{ steps.baseball-auth-ready.outputs.missing }} + run: | + echo "::error::Missing required secret(s): ${MISSING_BASEBALL_SECRETS}" + echo "BaseballHelm authenticated smoke (#372) is a required PR gate on this repo and needs these secrets configured." + exit 1 + + - name: Cache Playwright browsers + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('package-lock.json') }} + restore-keys: | + playwright-${{ runner.os }}- + + - name: Install Playwright browsers + run: npx playwright install --with-deps chromium + + - name: Build (so dev server boots fast) + run: npm run build + + - name: Seed BaseballHelm CI accounts + run: npm run seed:baseball:ci + + - name: Run BaseballHelm mandatory smoke (blocking) + run: | + npx playwright test \ + --project=chromium \ + --project=baseball-coach \ + --project=baseball-player \ + e2e/baseball-onboarding-smoke.spec.ts \ + e2e/baseball-smoke.spec.ts + + - name: Upload Playwright report + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a + with: + name: baseball-auth-smoke-playwright-report + path: playwright-report/ + retention-days: 3 + if-no-files-found: ignore + all: name: all if: always() @@ -321,6 +441,11 @@ jobs: # baseball pgTAP RLS suite was greened on main by #517 (supersedes #423), so # RLS regressions now block merge through the required `all` check instead of # being advisory-only — the gap that let the anon-access hole reach main. + # + # `baseball-auth-smoke` (BaseballHelm authenticated coach/player smoke, + # #372) is part of the hard gate for the same reason: an authenticated + # smoke suite that only ran post-merge could not have caught a real + # regression before it landed on `main`. needs: - database-types - schema-invariants @@ -334,6 +459,7 @@ jobs: - route-hygiene - supabase - import-cycles + - baseball-auth-smoke steps: - name: Fail if any required CI check failed if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml index 2cb114148..791373e82 100644 --- a/.github/workflows/playwright.yml +++ b/.github/workflows/playwright.yml @@ -173,10 +173,19 @@ jobs: e2e/baseball-smoke.spec.ts \ e2e/mobile-viewports.spec.ts + # NOTE: previously `|| echo "Playwright suite has failures..."`, which + # masked the exit code for every spec in this job — a real failure + # here looked identical to a clean pass in the GitHub Actions UI + # (flagged as blocking trust in the e2e gate on #812 review). This job + # ("Playwright (chromium)") is advisory, not a required check (see + # docs/CI_RUNBOOK.md / .github/branch-protection.md), so unmasking it + # cannot newly block a merge — it only makes this job's own status + # honestly reflect whether the suite passed. The report/results-json + # upload steps below still run regardless (`if: always()`). - name: Run Playwright tests env: PLAYWRIGHT_BASEBALL_SEEDED: ${{ env.SUPABASE_SERVICE_ROLE_KEY != '' && '1' || '' }} - run: timeout 45m npx playwright test --project=chromium || echo "Playwright suite has failures or timed out — see report artifact" + run: timeout 45m npx playwright test --project=chromium - name: Upload Playwright report if: always() diff --git a/docs/CI_RUNBOOK.md b/docs/CI_RUNBOOK.md index 9f2f8ad25..26e4e9843 100644 --- a/docs/CI_RUNBOOK.md +++ b/docs/CI_RUNBOOK.md @@ -18,14 +18,14 @@ rolls up into one of those. (The context name `all` is shared by both the | Check | Source | What it validates | Gate type | |---|---|---|---| -| `all` (CI) | `ci.yml` | aggregate: DB-types drift, schema invariants, feature knowledge, typecheck, ESLint, lint-ratchet, unit tests, business contracts, `next build`, route hygiene, **Supabase lint + RLS tests** | **Hard gate** (required context `all`) | +| `all` (CI) | `ci.yml` | aggregate: DB-types drift, schema invariants, feature knowledge, typecheck, ESLint, lint-ratchet, unit tests, business contracts, `next build`, route hygiene, **Supabase lint + RLS tests**, **BaseballHelm authenticated coach/player smoke (#372)** | **Hard gate** (required context `all`) | | `all` (Review Gate) | `review-gate.yml` | aggregate: ast-grep, semgrep, gitleaks, actionlint, yamllint, shellcheck, markdownlint, ruff+pylint, sqlfluff, hadolint | **Hard gate** (required context `all`) | | `Smoke checks` | `playwright.yml` (PRs + main push) | build-only smoke: `npm ci` + `next build` (no full E2E) | **Hard gate** | | `Playwright PR smoke (a11y)` | `pr-smoke.yml` | public-route accessibility Playwright only when frontend/e2e paths change | Advisory | | `CodeRabbit` | CodeRabbit GitHub App | assertive line-level review + blocking custom checks (`.coderabbit.yaml`) | **Hard gate** | | `CodeQL` | `codeql.yml` | code-scanning security analysis | **Hard gate** | | `Greptile Review` | Greptile GitHub App | whole-codebase review + hard rules (`.greptile/rules.md`) | Advisory — *not* a required context; Greptile skips `dependabot` PRs, so requiring it would block the bot flow. CodeRabbit is the blocking AI reviewer. | -| `Playwright (chromium)` / `Course picker screenshots` / `BaseballHelm seeded smoke` | `playwright.yml` | full E2E — **main push + manual `workflow_dispatch` only** (not PRs) | Advisory on main; manual for feature branches | +| `Playwright (chromium)` / `Course picker screenshots` / `BaseballHelm seeded smoke` | `playwright.yml` | full E2E (mandatory Baseball smoke + mobile-viewport regression + broader chromium suite) — **main push + manual `workflow_dispatch` only** (not PRs) | Advisory on main; manual for feature branches. **Note:** `Playwright (chromium)`'s broader-suite step no longer masks its exit code (`|| echo ...` removed) — a red run here now means a real failure, not just "see artifact." | | `ci/circleci: lighthouse-preview` | CircleCI | Lighthouse against the Vercel preview URL; usually skips when no preview exists (non-main Vercel builds disabled) | Advisory | | `ci/circleci: ios-compile` | CircleCI | iOS Capacitor compile, only relevant when `ios/**` / `capacitor.config.ts` changed | Advisory unless the PR touches iOS | | `migration-lockdown / block-historical-edits` | `migration-lockdown.yml` | blocks edits to already-applied migrations | Advisory | @@ -45,6 +45,13 @@ Don't treat a check as "stuck" before its normal window has passed: - **Full Playwright** (`playwright.yml`, main + manual only) — `e2e` job 75-minute budget; `picker-screenshots` and `baseball-smoke` 20 minutes each; main-push `Smoke checks` 15 minutes. +- **`CI / all`'s `baseball-auth-smoke` job (#372)** — 30-minute budget; on + every same-repo, non-Dependabot PR/push it installs Playwright chromium, + runs a full `npm run build`, seeds BaseballHelm CI accounts, then runs the + mandatory coach/player smoke. This is separate from — and in addition to — + the broader `Smoke checks` build. On fork/Dependabot PR runs it **skips** + (no repo secrets available) rather than running or failing; a skip here + is expected, not stuck. - **Web server / auth waits** (why Playwright can be slow to even start) — 120s dev-server startup, 45s auth navigation per spec. @@ -96,7 +103,9 @@ your diff — `main` itself was already red when you branched. - **Don't misread intentional skips as failures**: Lighthouse skips `docs/*` and `*-noop` branches by design; Playwright specs self-skip when their env vars aren't set (`PLAYWRIGHT_BASEBALL_SEEDED`, `E2E_GOLF_*`, - `GOLFHELM_*`). A skip is not a failure. + `GOLFHELM_*`). `CI / all`'s `baseball-auth-smoke` job (#372) similarly + skips (not fails) on fork/Dependabot `pull_request` runs, which never + receive repo secrets. A skip is not a failure. ---