From 3d425e1aa2e0af513fda8757cbd76e90d154f2bf Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Sat, 8 Aug 2026 16:26:33 +0800 Subject: [PATCH 1/2] ci(lighthouse): make the budget gate self-healing and scope it to perf-relevant diffs The Lighthouse budget check has been red on every pull request. It is a relative gate, and check-lighthouse-budget.mjs fails closed when a baseline row's chromeVersion differs from the measuring run's, because a browser bump is otherwise indistinguishable from an application regression. PR #1716 pinned Playwright's managed Chromium (HeadlessChrome/151) so the browser stops varying per runner, but the committed baseline still records HeadlessChrome/150 from the old ambient runner Chrome, so all ten cells fail "evidence incomplete" every run. Three changes, none of which weaken the verdict: - Extract the Chromium pin into .github/actions/setup-lighthouse-chromium and use it from both Lighthouse jobs. If the measuring and refreshing jobs resolve different browsers, a refreshed baseline records a browser other than the one grading against it, and the gate goes permanently red again. - Add a dispatch-only lighthouse-baseline-refresh job behind the new refresh_lighthouse_baseline workflow_dispatch input. The baseline is a browser-specific artefact, so it can only be produced on a runner; this job measures under the pinned Chromium and uploads a rewritten lighthouse-budget.json for a human to review and commit. It deliberately cannot push, is not continue-on-error, and prints the distinct chromeVersion values as its acceptance check. - Retry a cell that produced no measurement at all exactly once. The failure has two shapes and only one was detected: Lighthouse can also exit 0 and write a well-formed report whose only content is a runtimeError such as NO_NAVSTART, which is what /forms does intermittently (ledger #147). A run that never started measured nothing about the diff, but it is not a pass either, so if the retry also produces nothing the grader still fails closed. Every retry is reported to the run summary and written to retries.txt whether or not it recovered. scripts/lighthouse-measurement-outcome.mjs owns that decision so it is unit testable; a cell that did measure and produced bad numbers is never retried. Browser drift now collapses to one message naming the refresh dispatch instead of ten near-identical sentences, but only when drift is the whole story and every run drifted the same way. A measurement gap or a mixed set of browsers still lists per run. compareToLighthouseBudget still returns fail, independently of enforce. Separately, the job keyed off `ui_changed || build_changed`, which put every dependabot lockfile bump (#1668, js-yaml) and every worker/** ingestion change through a ~7 minute isolated production build plus ten Lighthouse runs. It now keys off a new, narrower perf_changed scope that excludes worker and container surfaces, dependency manifests, Playwright and test surfaces, src/app/api/** and src/app/mockups/**. An unrecognised path under a listed root stays in scope, so a future refactor over-triggers by one job rather than silently dropping a render surface. The lockfile exclusion is the one deliberate hole: ci-change-scope.mjs sees paths, never the lockfile diff, so it cannot tell a devDependency bump from a React/Next bump. It is covered by check:bundle-budget on the same pull requests and by the push-to-main arm of the job's condition. enforce stays false and the job stays out of pr-required; promoting it is ledger #118's separate staged work, and ci-cache-safety.test.ts now fails if that promotion happens without restoring merge_group to the job's condition. Co-Authored-By: Claude Opus 5 --- .../setup-lighthouse-chromium/action.yml | 49 ++++ .github/workflows/ci.yml | 155 ++++++++++--- docs/process-hardening.md | 24 +- docs/scripts-index.md | 2 +- docs/testing.md | 57 ++++- scripts/check-lighthouse-budget.mjs | 27 ++- scripts/ci-change-scope.mjs | 212 ++++++++++++++++++ scripts/lighthouse-measurement-outcome.mjs | 60 +++++ scripts/run-lighthouse-budget.mjs | 109 ++++++--- tests/check-lighthouse-budget.test.ts | 128 ++++++++++- tests/ci-cache-safety.test.ts | 82 +++++++ 11 files changed, 828 insertions(+), 77 deletions(-) create mode 100644 .github/actions/setup-lighthouse-chromium/action.yml create mode 100644 scripts/lighthouse-measurement-outcome.mjs diff --git a/.github/actions/setup-lighthouse-chromium/action.yml b/.github/actions/setup-lighthouse-chromium/action.yml new file mode 100644 index 0000000000..4e131508eb --- /dev/null +++ b/.github/actions/setup-lighthouse-chromium/action.yml @@ -0,0 +1,49 @@ +name: Setup pinned Chromium for Lighthouse +description: > + Install Playwright's managed Chromium and export PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH, so every + Lighthouse job in this repository measures with the same browser build for a given commit. + +# The Lighthouse budget used to drive whatever Chrome ships in the ubuntu-24.04 runner image +# (matching live-web-vitals.yml, which measures a live domain and isn't graded against a +# committed baseline). That ambient browser is NOT pinned per commit — the runner fleet was +# observed serving HeadlessChrome/150 and /151 to jobs minutes apart on 2026-08-07 — so the +# baseline comparator treated the version drift as "evidence incomplete" and failed closed +# (by design: a browser bump is otherwise indistinguishable from a real regression). +# Refreshing the baseline for whichever version showed up bought a few hours before the next +# mismatched runner failed it again (#1690). +# +# Pin Playwright's own managed Chromium instead, the same binary the Production UI / Visual +# baselines jobs already use, cached by lockfile hash so it is identical across every runner +# for a given commit. run-lighthouse-budget.mjs already honours +# CHROME_PATH/PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH. +# +# This lives in a composite action rather than being duplicated per job for one reason: the +# measuring job and the baseline-refresh job MUST resolve the same executable. If they drift, +# the refreshed baseline records a browser other than the one that will grade against it, and +# the gate goes permanently red again — exactly the failure this action exists to end. + +runs: + using: composite + steps: + - name: Restore Chromium browser cache + id: pw-cache + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 + with: + path: ~/.cache/ms-playwright + key: playwright-chromium-${{ runner.os }}-${{ hashFiles('package-lock.json') }} + restore-keys: | + playwright-chromium-${{ runner.os }}- + + - name: Install Chromium + shell: bash + run: | + if [ "${{ steps.pw-cache.outputs.cache-hit }}" = "true" ]; then + npx playwright install-deps chromium + npx playwright install chromium + else + npx playwright install --with-deps chromium + fi + + - name: Pin the Chromium executable path + shell: bash + run: echo "PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=$(node -e "console.log(require('playwright').chromium.executablePath())")" >> "$GITHUB_ENV" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c524f7cbd..c64cce3353 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,6 +8,15 @@ on: types: [opened, synchronize, reopened, ready_for_review] merge_group: workflow_dispatch: + inputs: + refresh_lighthouse_baseline: + description: > + Re-measure the budgeted routes on this runner's PINNED Chromium and upload a refreshed + lighthouse-budget.json as an artifact for a human to review and commit. The baseline is a + browser-specific artefact, so it can only be produced on a runner — never from a developer + machine. Nothing is committed or pushed by this run. + type: boolean + default: false schedule: - cron: "0 18 * * 0" @@ -39,6 +48,7 @@ jobs: static_heavy_changed: ${{ steps.scope.outputs.static_heavy_changed }} coverage_changed: ${{ steps.scope.outputs.coverage_changed }} ui_changed: ${{ steps.scope.outputs.ui_changed }} + perf_changed: ${{ steps.scope.outputs.perf_changed }} advisory_ui_changed: ${{ steps.scope.outputs.advisory_ui_changed }} db_changed: ${{ steps.scope.outputs.db_changed }} container_changed: ${{ steps.scope.outputs.container_changed }} @@ -706,7 +716,54 @@ jobs: lighthouse-budget: name: Lighthouse budget (advisory) needs: changes - if: needs.changes.outputs.ui_changed == 'true' || needs.changes.outputs.build_changed == 'true' + # Runs when the diff can plausibly move LCP/TBT/CLS on the five budgeted routes, + # and when the run is one whose result someone will act on. It used to key off + # `ui_changed || build_changed`, which put every dependabot lockfile bump (#1668) + # and every worker/** ingestion change through a ~7 minute isolated production + # build plus ten Lighthouse runs, with zero render-path relevance. + # + # pull_request – perf scope, non-draft. Drafts churn and this costs ~7 min; + # `types:` already includes ready_for_review, so undrafting + # re-triggers it without a push. + # merge_group – SKIPPED. The job is continue-on-error and is not in + # pr-required, so in the queue it can only add ~7 minutes of + # merge latency and can never change the outcome. If it is + # ever promoted (#118), merge_group MUST come back — + # tests/ci-cache-safety.test.ts enforces that pairing. + # push (main/…) – always on perf scope. This lane catches what the PR arm + # deliberately does not: a runtime dependency bump, which + # perf scope excludes because ci-change-scope.mjs sees paths, + # not lockfile contents. + # schedule – weekly; resolves to the full-run sentinel, which trips perf + # scope (asserted in ci-change-scope.mjs's self-tests). + # workflow_dispatch – always, except a run dispatched to refresh the baseline + # (that belongs to lighthouse-baseline-refresh below). + # + # Labels mirror the codex-review / skip-codex-review convention; an explicit skip + # always wins. `on.pull_request.types` has no `labeled`, so the opt-in label takes + # effect on the next synchronize or re-run — adding `labeled` would re-run all of + # CI on every label change, so the escape hatch is workflow_dispatch instead. + # + # Two expression traps, both load-bearing: + # - `github.event.inputs.X` is a STRING ('true'/'false') and is safely null on + # non-dispatch events; the `inputs` context is a real boolean but only exists + # for workflow_dispatch/workflow_call. Use the github.event.inputs form. + # - `github.event.pull_request` is null on push/schedule/merge_group, so + # `draft != true` is true there. Do NOT write `== false`: that is false on + # every non-PR event and would silently kill the push and schedule arms. + if: > + ( + github.event_name == 'schedule' || + github.event_name == 'workflow_dispatch' || + contains(github.event.pull_request.labels.*.name, 'lighthouse-budget') || + ( + needs.changes.outputs.perf_changed == 'true' && + github.event_name != 'merge_group' && + github.event.pull_request.draft != true + ) + ) && + !contains(github.event.pull_request.labels.*.name, 'skip-lighthouse-budget') && + github.event.inputs.refresh_lighthouse_baseline != 'true' continue-on-error: true runs-on: ubuntu-24.04 timeout-minutes: 45 @@ -719,40 +776,8 @@ jobs: - name: Setup Node and dependencies uses: ./.github/actions/setup-node-cached - # This job used to drive whatever Chrome ships in the ubuntu-24.04 runner image - # (matching live-web-vitals.yml, which measures a live domain and isn't graded - # against a committed baseline). That ambient browser is NOT pinned per commit — - # the runner fleet was observed serving HeadlessChrome/150 and /151 to jobs - # minutes apart on 2026-08-07 — so every grading run this job's baseline - # comparator treats a version drift as "evidence incomplete" and fails closed - # (by design: a browser bump is otherwise indistinguishable from a real - # regression). Refreshing the baseline for whichever version showed up bought a - # few hours before the next mismatched runner failed it again (#1690). - # - # Pin Playwright's own managed Chromium instead, the same binary the Production - # UI / Visual baselines jobs already use, cached by lockfile hash so it is - # identical across every runner for a given commit. run-lighthouse-budget.mjs - # already honours CHROME_PATH/PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH. - - name: Restore Chromium browser cache - id: pw-cache - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 - with: - path: ~/.cache/ms-playwright - key: playwright-chromium-${{ runner.os }}-${{ hashFiles('package-lock.json') }} - restore-keys: | - playwright-chromium-${{ runner.os }}- - - - name: Install Chromium - run: | - if [ "${{ steps.pw-cache.outputs.cache-hit }}" = "true" ]; then - npx playwright install-deps chromium - npx playwright install chromium - else - npx playwright install --with-deps chromium - fi - - - name: Pin the Chromium executable path - run: echo "PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=$(node -e "console.log(require('playwright').chromium.executablePath())")" >> "$GITHUB_ENV" + - name: Setup pinned Chromium + uses: ./.github/actions/setup-lighthouse-chromium - name: Measure routes and grade against the baseline run: npm run verify:lighthouse -- --keep --dir lighthouse @@ -765,6 +790,66 @@ jobs: path: lighthouse/ if-no-files-found: ignore + # Refresh the committed Lighthouse baseline FROM A CI RUNNER. Dispatch-only. + # + # The baseline is a browser-specific artefact: check-lighthouse-budget.mjs fails + # closed when a baseline row's chromeVersion differs from the measuring run's, + # because a browser bump is otherwise indistinguishable from an application + # regression. So the numbers must come from the same pinned Chromium the grading job + # uses, which a developer machine cannot supply (#118: never commit baselines from a + # developer machine). This job produces the file; a human reviews the diff and + # commits it. That division is the point — a workflow that can rewrite a gate's own + # baseline is a gate that can green itself. + # + # Deliberately NOT continue-on-error: a refresh that measured nothing must go red. + lighthouse-baseline-refresh: + name: Refresh Lighthouse baseline (dispatch only) + if: github.event_name == 'workflow_dispatch' && github.event.inputs.refresh_lighthouse_baseline == 'true' + runs-on: ubuntu-24.04 + timeout-minutes: 45 + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Setup Node and dependencies + uses: ./.github/actions/setup-node-cached + + - name: Setup pinned Chromium + uses: ./.github/actions/setup-lighthouse-chromium + + # --update refuses on a measurement gap (incompleteBudgetEvidence with + # ignoreBaseline:true still blocks a missing or unusable report) and accepts + # browser drift, which is the reason to refresh. --keep retains the raw reports + # so the artifact carries evidence, not just a conclusion. + - name: Measure and rewrite the baseline + run: npm run verify:lighthouse -- --update --keep --dir lighthouse + + # --update returns before rendering the table, so without this nothing describes + # what was recorded. Re-grading the same reports against the just-written + # baseline yields zero deltas by construction: this RENDERS the recorded numbers + # into the step summary, it does not verify them. + - name: Render the recorded baseline into the run summary + run: npm run check:lighthouse-budget -- --dir lighthouse + + # The acceptance check for this job: exactly one distinct browser, and it is the + # pinned one. More than one line here means the refresh is not usable. + - name: Show the baseline diff and the browser it was measured on + run: | + git --no-pager diff --stat -- lighthouse-budget.json + node -e "const b=require('./lighthouse-budget.json');console.log([...new Set(Object.values(b.baseline??{}).map(r=>r.chromeVersion))].join('\n'))" + + - name: Upload the refreshed baseline + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: lighthouse-baseline-refresh-${{ github.run_id }} + path: | + lighthouse-budget.json + lighthouse/ + if-no-files-found: error + db-reset-verify: name: Migration replay needs: changes diff --git a/docs/process-hardening.md b/docs/process-hardening.md index 1adb19edb6..5857a5422a 100644 --- a/docs/process-hardening.md +++ b/docs/process-hardening.md @@ -301,10 +301,28 @@ Three gates added for the "mature repo" verification pass. Full usage is in - **The performance budget is relative, not absolute.** `lighthouse-budget.json` holds a committed per-route baseline and a per-metric tolerance, following `check:bundle-budget`. Absolute web-vitals thresholds are meaningless against localhost with no network latency. `enforce` starts - `false` with no baseline; incomplete evidence fails regardless of `enforce`. + `false`; incomplete evidence fails regardless of `enforce`. +- **The baseline is browser-specific, and the browser is now pinned (2026-08-08).** Each baseline row + records the `chromeVersion` that produced it, and a mismatch fails closed — a browser bump is + otherwise indistinguishable from an application regression. The ambient runner-image Chrome is not + pinned per commit (the fleet served HeadlessChrome/150 and /151 to jobs minutes apart on + 2026-08-07), so both Lighthouse jobs resolve Playwright's managed Chromium through one shared + composite action, `./.github/actions/setup-lighthouse-chromium`. Refreshing is a `workflow_dispatch` + input (`refresh_lighthouse_baseline`) that uploads a rewritten `lighthouse-budget.json` for a human + to review and commit; it deliberately cannot push, because a workflow that rewrites a gate's own + baseline is a gate that can green itself. +- **The budget runs on perf scope, not the ui/build union (2026-08-08).** `perf_changed` in + `scripts/ci-change-scope.mjs` excludes `worker/**` and container surfaces, dependency manifests and + the lockfile, Playwright/test surfaces, `src/app/api/**` and `src/app/mockups/**`. The lockfile + exclusion is the one deliberate hole: the classifier sees paths, never the lockfile diff, so it + cannot tell a devDependency bump from a React/Next bump. It is covered by `check:bundle-budget` on + the same PRs and by the `push`-to-`main` arm of the job's `if:`. If a runtime dep bump ever lands an + unnoticed LCP regression, teach the classifier to read the lockfile diff — do not put the whole + lockfile back into perf scope. - **Verification debt remaining.** (1) No pixel baselines are committed, so `visual-baseline` reports - rather than gates until an operator adopts them from the CI artifact. (2) No Lighthouse baseline is - recorded, so the budget warns rather than grades until `--update` runs against a known-good build. + rather than gates until an operator adopts them from the CI artifact. (2) A Lighthouse baseline is + committed, but `enforce` stays `false` until #147 (`/dsm` mobile CLS 0.363) and #117 are resolved — + enforcing today would ratify those breaches. (3) 37 of the 38 unlayered visual classes carry exemptions rather than effect contracts — the debt is enumerated in `STYLE_CONTRACT_EXEMPTIONS`, and ledger #094 stays open until the load-bearing ones have contracts. (4) `scripts/run-lighthouse-budget.mjs` duplicates roughly 50 lines of the diff --git a/docs/scripts-index.md b/docs/scripts-index.md index d24c8c5a17..411ad1216f 100644 --- a/docs/scripts-index.md +++ b/docs/scripts-index.md @@ -1,6 +1,6 @@ # Scripts index -Curated map of `scripts/` (218 files) and the `package.json` script surface (231 entries), +Curated map of `scripts/` (219 files) and the `package.json` script surface (231 entries), grouped by purpose. This is orientation, not an exhaustive per-file listing — the authoritative command list is `package.json`, and `npm run docs:check-scripts` verifies every `npm run ` referenced in docs resolves to a real script. `npm run docs:update` refreshes the exact counts above. diff --git a/docs/testing.md b/docs/testing.md index 6801fb9272..3f295b74d4 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -133,8 +133,59 @@ following the same shape as `check:bundle-budget`. route counted as a pass is the failure mode `summarise-web-vitals.mjs` documents at length. - CLS is graded on absolute movement; LCP and TBT need to clear both a percentage and an absolute floor, so 12 ms → 16 ms is not reported as a 33% regression. - -Refresh the baseline deliberately after a known-good run: `npm run check:lighthouse-budget -- --update`. +- A cell that produced **no measurement at all** — a non-zero exit, no report file, or a report + carrying only a `runtimeError` such as Lighthouse's own `NO_NAVSTART` (ledger #147 recorded + `/forms` doing this) — is retried **once**, and the retry is always logged to the run summary and + written to `retries.txt` whether or not it recovered. A run that never started measured nothing + about the diff; it is not a pass either, so if the retry also produces nothing the grader still + fails closed. A cell that _did_ measure and produced bad numbers is never retried. + +### Baseline browser pinning, and how to refresh + +The baseline is a **browser-specific** artefact. `check-lighthouse-budget.mjs` fails closed when a +baseline row's `chromeVersion` differs from the measuring run's, because a browser bump is otherwise +indistinguishable from an application regression. Both Lighthouse jobs therefore pin Playwright's +managed Chromium through the shared `./.github/actions/setup-lighthouse-chromium` composite action — +never the ambient runner-image Chrome, which is not pinned per commit (the fleet was observed serving +HeadlessChrome/150 and /151 to jobs minutes apart on 2026-08-07). Drift is reported as one collapsed +instruction rather than one sentence per route, but the verdict is unchanged: incomplete evidence +still fails, independently of `enforce`. + +Because the numbers must come from that pinned browser, refresh the baseline **from a CI runner**, +never a developer machine: + +1. Actions → CI → **Run workflow** → pick the branch → tick **refresh_lighthouse_baseline** → Run. +2. Check the run's diff step prints exactly **one** distinct `chromeVersion`, and that it is the + pinned `HeadlessChrome/`. More than one line means the refresh is not usable. +3. Download the `lighthouse-baseline-refresh-` artifact, review the per-route deltas, and + commit **only** `lighthouse-budget.json`. + +The refresh job is dispatch-only, is not `continue-on-error` (a refresh that measured nothing must go +red), and deliberately cannot push — a workflow that can rewrite a gate's own baseline is a gate that +can green itself. `npm run check:lighthouse-budget -- --update` exists for local experiments; its +output must not be committed. + +### When the budget runs + +Keyed off `perf_changed` (`scripts/ci-change-scope.mjs`), which is deliberately narrower than the +`ui_changed || build_changed` union it replaced — that union put every dependabot lockfile bump and +every `worker/**` ingestion change through a ~7 minute isolated production build plus ten Lighthouse +runs. In scope: `src/**`, `data/**`, `public/**`, `next.config.ts`, `postcss.config.mjs`, +`tsconfig.json`, and the budget's own inputs. Excluded: `worker/**` and container surfaces, dependency +manifests and the lockfile, Playwright/test surfaces including committed screenshots, +`src/app/api/**`, `src/app/mockups/**`, and the server/edge runtime entry points. An unrecognised path +under a listed root stays **in** scope, so a future refactor over-triggers by one job rather than +silently dropping a render surface. + +Event matrix: pull requests on perf scope when not a draft; `merge_group` skipped while the job is +advisory (it could only add merge latency without changing the outcome — promoting it to +`pr-required` must restore `merge_group` in the same edit, which `tests/ci-cache-safety.test.ts` +enforces); `push` to `main`/`release/**` always on perf scope, which is the lane that catches a +runtime dependency bump the PR arm deliberately excludes; the weekly `schedule`; and +`workflow_dispatch`. The `lighthouse-budget` label forces a run and `skip-lighthouse-budget` opts out, +with skip winning. Caveat: `on.pull_request.types` has no `labeled` (adding it would re-run all of CI +on every label change), so the opt-in label takes effect on the next push or re-run — use +`workflow_dispatch` for an immediate run. This is distinct from `.github/workflows/live-web-vitals.yml`, which measures the deployed origin for ledger #017 and is dispatch-only — by the time it runs, `main` has already auto-deployed. Both pin @@ -153,7 +204,7 @@ UI scope runs a fail-fast `@critical` Chromium job on pull requests, then one re PR body synchronization is skipped unless the checked-out head actually contains `PR_POLICY_BODY.md`. The eval-canary liveness API probe runs once with the daily Ops Digest cadence rather than on every PR. These remove repeated provider-side work without weakening a required result. -Two further jobs are advisory (`continue-on-error`, deliberately outside `pr-required`): `visual-baseline` on UI scope and `lighthouse-budget` on UI-or-build scope. Both upload their evidence on every run, pass or fail, because the artifact is the whole point on a first run — the baselines to adopt and the reports to grade. Promote either to required by adding it to `pr-required` and removing `continue-on-error` in the same edit. +Two further jobs are advisory (`continue-on-error`, deliberately outside `pr-required`): `visual-baseline` on UI scope and `lighthouse-budget` on the narrower perf scope (see "When the budget runs" above — `worker/**` and container surfaces, dependency manifests and the lockfile, Playwright/test surfaces, `src/app/api/**` and `src/app/mockups/**` are all excluded). Both upload their evidence on every run, pass or fail, because the artifact is the whole point on a first run — the baselines to adopt and the reports to grade. Promote either to required by adding it to `pr-required` and removing `continue-on-error` in the same edit; for `lighthouse-budget` that edit must also restore `merge_group` to its `if:`. ## Contribution checklist (UI changes) diff --git a/scripts/check-lighthouse-budget.mjs b/scripts/check-lighthouse-budget.mjs index bc549bd45a..e175baa691 100644 --- a/scripts/check-lighthouse-budget.mjs +++ b/scripts/check-lighthouse-budget.mjs @@ -87,6 +87,10 @@ export function incompleteBudgetEvidence(rows, budget, { ignoreBaseline = false // before anything is measured rather than a per-run problem. const problems = new Set(collidingRouteSlugs(budget?.routes ?? []).map((slug) => `route slug collision: ${slug}`)); const byRun = new Map(rows.map((row) => [row.run, row])); + // Browser drift is collected separately from the other problems because it is ONE + // fact about the baseline, not N independent per-route defects — see the collapse + // below. The verdict is identical either way; only the message changes. + const drift = new Map(); for (const run of expectedBudgetRuns(budget)) { const row = byRun.get(run); @@ -123,11 +127,28 @@ export function incompleteBudgetEvidence(rows, budget, { ignoreBaseline = false // version, so a browser bump is otherwise indistinguishable from an application // regression. summarise-web-vitals.mjs makes the same point about its baselines. if (before.chromeVersion && row.chromeVersion && before.chromeVersion !== row.chromeVersion) { - problems.add( - `${run}: baseline measured by a different browser (${before.chromeVersion} vs ${row.chromeVersion}) — refresh with --update`, - ); + drift.set(run, { before: before.chromeVersion, after: row.chromeVersion }); } } + + // One browser bump reds every run in the budget, and printing ten near-identical + // sentences buried the single actionable instruction. Collapse to one line ONLY + // when drift is the whole story and every run drifted the same way — any + // measurement gap, or a mixed set of browsers, still lists per run because those + // are genuinely different facts. This changes the message, never the verdict: + // `compareToLighthouseBudget` still returns `fail` on a non-empty result, + // independently of `enforce`. + const driftPairs = new Set([...drift.values()].map(({ before, after }) => JSON.stringify([before, after]))); + if (drift.size > 0 && problems.size === 0 && driftPairs.size === 1) { + const [{ before, after }] = drift.values(); + return [ + `browser drift on ${drift.size} run(s): the baseline was measured by ${before}, this run used ${after} — ` + + 'refresh it with the CI "Refresh Lighthouse baseline" dispatch (workflow_dispatch, refresh_lighthouse_baseline)', + ]; + } + for (const [run, { before, after }] of drift) { + problems.add(`${run}: baseline measured by a different browser (${before} vs ${after}) — refresh with --update`); + } return [...problems].sort(); } diff --git a/scripts/ci-change-scope.mjs b/scripts/ci-change-scope.mjs index 054fbe7b8d..9ffe495097 100644 --- a/scripts/ci-change-scope.mjs +++ b/scripts/ci-change-scope.mjs @@ -24,6 +24,7 @@ const outputs = [ "static_heavy_changed", "coverage_changed", "ui_changed", + "perf_changed", "advisory_ui_changed", "db_changed", "container_changed", @@ -170,6 +171,71 @@ const uiPatterns = [ /^scripts\/(run|check)-lighthouse-budget\.mjs$/, ]; +/** + * perf_changed — surfaces that can plausibly move LCP / TBT / CLS on the five routes + * `lighthouse-budget.json` measures. Deliberately NARROWER than the `ui_changed || + * build_changed` union the budget job used to key off: that union put a dev-dependency + * lockfile bump (#1668, js-yaml) and every `worker/**` ingestion change through a ~7 + * minute isolated `next build --webpack` plus ten Lighthouse runs, with zero + * render-path relevance. + * + * Direction of failure is deliberate: an UNRECOGNISED path under a listed root is IN + * scope. Adding `src/features/` later over-triggers by one job rather than silently + * dropping a render surface out of the perf gate. + */ +const perfPatterns = [ + // Every measured route is a segment of src/app, and the root layout, the + // (search-app) group layout and both CSS entry points ship on all five. src/lib is + // NOT split into server/client here: only 13 of ~256 files carry `import + // "server-only"`, and src/lib/supabase/client.tsx is a browser provider in the + // render tree, so a path-based split would fail open. + "src", + // Route payload, both forms: data/** is imported into route chunks and public/** is + // fetched on the critical path (#117 therapies-home ~136 KB, #013 forms-catalog + // ~132 KB). + "data", + "public", + // These rewrite the emitted bundle/CSS for every route, so a change invalidates the + // whole baseline rather than one route. + "next.config.ts", + "postcss.config.mjs", + "tsconfig.json", + // The budget's own inputs. summarise-web-vitals.mjs is here because + // check-lighthouse-budget.mjs imports summariseReport / hasUsableMetrics / + // measuredRequestedPage from it — editing it changes the VERDICT, and before this + // it matched no CI scope pattern at all. + "lighthouse-budget.json", + /^scripts\/(run-lighthouse-budget|check-lighthouse-budget|summarise-web-vitals|lighthouse-measurement-outcome)\.mjs$/, +]; + +/** + * Paths inside a `perfPatterns` root that cannot reach a measured route's render + * path. Each is a deliberate loss of coverage; a path NOT listed here stays in scope. + */ +const perfExclusionPatterns = [ + // App Router API handlers. No measured route issues an API request during load: + // /documents/search is a static server-rendered shell, and every budget route is + // measured without a query string. assertBudgetRoutesAreQueryFree() fails closed if + // that ever changes. Precedent: isUiChangedPath already excludes API routes. + "src/app/api", + // Separate route segments, 404 in production, and the runner sets + // NEXT_PUBLIC_MOCKUPS_ENABLED=false. Tailwind does scan src/**, so a utility class + // used only by a mockup adds bytes to the shared stylesheet — tens of bytes against + // an lcpMs floor of +100ms, and check:bundle-budget still enforces gzip growth. + "src/app/mockups", + // Server/edge runtime only. src/instrumentation-client.ts is deliberately NOT here: + // it executes in the browser and is a direct TBT contributor. + "src/instrumentation.ts", + "src/proxy.ts", + "src/sentry.server.config.ts", + "src/sentry.edge.config.ts", +]; + +function isPerfChangedPath(filePath) { + if (pathMatches(filePath, perfExclusionPatterns)) return false; + return pathMatches(filePath, perfPatterns); +} + // Migration replay validates schema/SQL tooling, not every API handler. API // route edits stay covered by unit/coverage (+ RAG offline when rag-scoped). const dbPatterns = [ @@ -279,6 +345,7 @@ function classify(files, { readLedger = readFlakeLedger, prPolicyBodyPresent = e const docsChanged = normalized.some((file) => pathMatches(file, docPatterns)); const sourceChanged = normalized.some((file) => pathMatches(file, [...sourcePatterns, ...staticConfigPatterns])); const uiChanged = normalized.some((file) => isUiChangedPath(file)); + const perfChanged = normalized.some((file) => isPerfChangedPath(file)); const advisoryUiChanged = normalized.some((file) => pathMatches(file, mockupPatterns)) || quarantineLedgerHasEntries(readLedger); const dbChanged = normalized.some((file) => pathMatches(file, dbPatterns)); @@ -316,6 +383,7 @@ function classify(files, { readLedger = readFlakeLedger, prPolicyBodyPresent = e static_heavy_changed: staticHeavyChanged, coverage_changed: coverageChanged, ui_changed: uiChanged, + perf_changed: perfChanged, advisory_ui_changed: advisoryUiChanged, db_changed: dbChanged, container_changed: containerChanged, @@ -530,6 +598,45 @@ function assertMockupSpecParity() { console.log(`Mockup spec parity: ${specs.length} advisory specs all match mockupPatterns.`); } +/** + * `perfExclusionPatterns` drops `src/app/api/**` on one measured fact: every route in + * `lighthouse-budget.json` is requested as a bare URL, so none of them issues an API + * request during load. A route gaining a query string (`/documents/search?q=…`) would + * silently invalidate that, and the resulting miss is exactly the kind that shows a + * green PR while the perf gate never ran against the code it depends on. + * + * So the assumption is guarded rather than hoped for. Fails CLOSED on an unreadable + * budget, like the mockup guard above. + */ +function assertBudgetRoutesAreQueryFree() { + let budget; + try { + budget = JSON.parse(readFileSync("lighthouse-budget.json", "utf8")); + } catch (error) { + throw new Error( + `budget-routes-query-free: could not read lighthouse-budget.json (${ + error instanceof Error ? error.message : String(error) + }). If that file moved, update this guard — do not delete it.`, + ); + } + + const routes = Array.isArray(budget?.routes) ? budget.routes : null; + if (!routes || routes.length === 0) { + throw new Error( + "budget-routes-query-free: lighthouse-budget.json lists no routes; this guard has nothing to check.", + ); + } + + const withQuery = routes.filter((route) => typeof route === "string" && route.includes("?")); + if (withQuery.length > 0) { + throw new Error( + `budget-routes-query-free: ${withQuery.join(", ")} carries a query string. A query-bearing budget route may ` + + "fetch src/app/api/** on load, so remove the `src/app/api` entry from perfExclusionPatterns or drop the route.", + ); + } + console.log(`Budget routes query-free: ${routes.length} routes carry no query string.`); +} + function selfTest() { // #137: the advisory lane runs only when it has something to cover. All four // directions matter — a lane that silently never runs is the failure mode. @@ -674,6 +781,106 @@ function selfTest() { ui_changed: true, }, ); + + // ---- perf_changed: the narrow scope the Lighthouse budget job keys off. ---- + // + // Both directions are load-bearing. A false negative means a render regression + // reaches main unmeasured; a false positive is the ~7 minute build+measure this + // scope exists to stop paying. Each `perf-off-*` case below is a deliberate, + // argued loss of coverage, not an oversight — see perfExclusionPatterns. + + // #1668 (dependabot js-yaml, a devDependency) paid the full budget run. This + // classifier only ever sees PATHS, never the lockfile diff, so it cannot tell a + // devDependency bump from a React/Next bump. The gap is covered by + // check:bundle-budget (enforced on the same PRs), by the push-to-main arm of the + // job's `if:`, and by the weekly schedule — NOT by putting the lockfile back here. + assertScope("perf-off-for-dependency-manifests", ["package.json", "package-lock.json"], { + build_changed: true, + lockfile_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-worker", ["worker/main.ts", "worker/python/requirements.txt"], { + build_changed: true, + container_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-container-surfaces", ["Dockerfile.worker", "railway.worker.json"], { + container_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-api-route", ["src/app/api/answer/route.ts"], { + build_changed: true, + ui_changed: false, + perf_changed: false, + }); + // A spec, fixture, golden PNG or browser config cannot change a byte the production + // server sends, and the Lighthouse runner builds and serves its own isolated app + // without ever loading Playwright. + assertScope( + "perf-off-for-playwright-surfaces", + [ + "tests/ui-smoke.spec.ts", + "tests/helpers/zero-touch.ts", + "tests/__screenshots__/linux/dashboard-shell.png", + "playwright.config.ts", + "scripts/run-playwright.mjs", + ], + { ui_changed: true, perf_changed: false }, + ); + assertScope("perf-off-for-mockup-route", ["src/app/mockups/page.tsx"], { + ui_changed: true, + advisory_ui_changed: true, + perf_changed: false, + }); + assertScope( + "perf-off-for-server-runtime-entrypoints", + ["src/instrumentation.ts", "src/proxy.ts", "src/sentry.server.config.ts", "src/sentry.edge.config.ts"], + { build_changed: true, perf_changed: false }, + ); + assertScope("perf-off-for-bundle-budget-config", ["bundle-budget.json"], { + build_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-docs", ["docs/testing.md"], { docs_only: true, perf_changed: false }); + assertScope("perf-off-for-supabase", ["supabase/migrations/20260101000000_example.sql"], { + db_changed: true, + perf_changed: false, + }); + + assertScope("perf-on-for-route-page", ["src/app/(search-app)/dsm/page.tsx"], { + ui_changed: true, + perf_changed: true, + }); + assertScope("perf-on-for-shared-component", ["src/components/clinical-dashboard/dashboard-nav.tsx"], { + perf_changed: true, + }); + // Pins the exclusion that was CONSIDERED AND REJECTED: src/lib is not split into + // server/client by path, because this file is a browser Supabase provider that sits + // in the render tree while its siblings are server-only. + assertScope("perf-on-for-browser-supabase-client", ["src/lib/supabase/client.tsx"], { perf_changed: true }); + // The mirror image: instrumentation-client.ts runs in the browser and is a direct + // TBT contributor, so the `src/instrumentation.ts` exclusion must not over-reach. + assertScope("perf-on-for-client-instrumentation", ["src/instrumentation-client.ts"], { perf_changed: true }); + assertScope("perf-on-for-css-entrypoints", ["src/app/globals.css"], { perf_changed: true }); + assertScope( + "perf-on-for-route-payload", + ["public/therapy-compass-data/therapies-home.json", "data/medications-snapshot.json"], + { perf_changed: true }, + ); + assertScope("perf-on-for-build-config", ["next.config.ts", "postcss.config.mjs", "tsconfig.json"], { + perf_changed: true, + }); + assertScope("perf-on-for-budget-inputs", ["lighthouse-budget.json"], { ui_changed: true, perf_changed: true }); + // New coverage: the grader imports its completeness primitives from this file, so + // editing it changes the verdict. Before perfPatterns it matched no scope at all. + assertScope("perf-on-for-grader-dependency", ["scripts/summarise-web-vitals.mjs"], { perf_changed: true }); + assertScope("perf-on-for-retry-outcome-module", ["scripts/lighthouse-measurement-outcome.mjs"], { + perf_changed: true, + }); + // Proves the unknown-path direction: a new top-level directory under src/ is IN + // scope, so a future refactor over-triggers by one job rather than silently + // dropping a render surface. + assertScope("perf-on-for-unrecognised-src-path", ["src/features/new-thing/index.ts"], { perf_changed: true }); assertScope("runtime-data", ["data/medications-snapshot.json"], { source_changed: true, coverage_changed: true, @@ -881,6 +1088,10 @@ function selfTest() { static_heavy_changed: true, coverage_changed: true, ui_changed: true, + // The weekly schedule and any unresolvable base resolve to these sentinels, and + // the perf gate's `if:` relies on that to keep measuring routes when no PR does. + // Asserted so a future edit to fullRunSentinelFiles cannot silently retire it. + perf_changed: true, db_changed: true, container_changed: true, rag_eval_changed: true, @@ -907,6 +1118,7 @@ function selfTest() { const args = process.argv.slice(2); if (args.includes("--self-test")) { assertMockupSpecParity(); + assertBudgetRoutesAreQueryFree(); selfTest(); process.exit(0); } diff --git a/scripts/lighthouse-measurement-outcome.mjs b/scripts/lighthouse-measurement-outcome.mjs new file mode 100644 index 0000000000..8c4b46ca96 --- /dev/null +++ b/scripts/lighthouse-measurement-outcome.mjs @@ -0,0 +1,60 @@ +/** + * lighthouse-measurement-outcome — decide whether a Lighthouse cell produced + * gradeable evidence at all, and therefore whether one retry is warranted. + * + * Why this is its own module: `run-lighthouse-budget.mjs` executes its whole pipeline + * at import time (it builds, serves and measures at the top level), so it cannot be + * imported by a test. Putting the decision here keeps it unit-testable without the + * source-text assertions this repo has been retiring. + * + * The distinction that matters: + * + * - A measurement that never STARTED is not evidence of a regression. Lighthouse's + * own `NO_NAVSTART` runtime error means the navigation never began, and its own + * advice is to run again; ledger #147 recorded `/forms` doing exactly this while + * the live dispatch measured it fine. + * - A measurement that DID start and produced bad numbers is a real measurement. + * It is never retried — that would be the gate re-rolling the dice until green. + * + * This only decides whether to retry. It never decides whether a run passes: the + * grader's `incompleteBudgetEvidence` still fails closed on a missing or unusable + * report regardless of `enforce`, before and after any retry. + * + * Note the second failure shape, which the runner previously missed entirely: + * Lighthouse exits 0 and writes a perfectly well-formed report whose only content is + * a `runtimeError`. That file satisfies "a report exists" while carrying no metrics, + * so an exit-code check alone leaves it unretried. + */ + +/** + * Why this cell produced no gradeable evidence, or `null` when it did. + * + * @param {number | null | undefined} exitCode Lighthouse's process exit code. + * @param {string | null | undefined} reportText Raw contents of the report file, or + * `null`/`undefined` when no file was written. + * @returns {string | null} A short human-readable reason, or `null` when the cell + * measured something real (including something real and slow). + */ +export function measurementFailureReason(exitCode, reportText) { + if (typeof exitCode !== "number" || exitCode !== 0) { + return `lighthouse exited ${exitCode ?? "without a status"}`; + } + if (reportText === null || reportText === undefined || reportText === "") { + return "no report file was written"; + } + + let parsed; + try { + parsed = JSON.parse(reportText); + } catch { + return "report is not valid JSON"; + } + + const code = parsed?.runtimeError?.code; + // Any runtimeError, not just NO_NAVSTART: `measuredRequestedPage` in + // summarise-web-vitals.mjs already rejects every report carrying one, so each is a + // cell that produced no comparable numbers. + if (typeof code === "string" && code.length > 0) return `lighthouse runtimeError ${code}`; + + return null; +} diff --git a/scripts/run-lighthouse-budget.mjs b/scripts/run-lighthouse-budget.mjs index 9e8ff1ce83..5eede6b2c6 100644 --- a/scripts/run-lighthouse-budget.mjs +++ b/scripts/run-lighthouse-budget.mjs @@ -21,11 +21,20 @@ * live workflow, which is grading a flaky public network): a route that produced no * report is incomplete evidence, and the grader fails closed on it. * + * A cell that produced no measurement AT ALL — a non-zero exit, no report file, or a + * report carrying only a `runtimeError` such as Lighthouse's own `NO_NAVSTART` — gets + * exactly ONE announced retry first (`lighthouse-measurement-outcome.mjs` draws that + * line). That is not a softening: a run that never started measured nothing about this + * diff, and if the retry also produces nothing the grader still fails closed. Every + * retry is reported to the run summary and written to `retries.txt` whether or not it + * recovered, so a chronically flaky route cannot hide behind a green run. A cell that + * DID measure and produced bad numbers is never retried. + * * Flags: --dry-run (print the plan and exit), --update (refresh the baseline), * --keep (leave reports in place), --dir . */ import { spawn, spawnSync } from "node:child_process"; -import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { appendFileSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import http from "node:http"; import net from "node:net"; import path from "node:path"; @@ -35,6 +44,7 @@ import { childProcessExitCode, childProcessFailureSummary } from "./child-proces import { offlineTestEnvironment } from "./test-environment.mjs"; import { acquireHeavyRunLock } from "./test-run-lock.mjs"; import { loadBudget } from "./check-lighthouse-budget.mjs"; +import { measurementFailureReason } from "./lighthouse-measurement-outcome.mjs"; import { appName, circularProjectPortRange, @@ -312,42 +322,83 @@ try { const chromePath = process.env.CHROME_PATH ?? process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH ?? ""; const failures = []; + const retried = []; + + const measure = (strategy, route, output) => + spawnSync( + npxInvocation.command, + [ + ...npxInvocation.prefixArgs, + "--yes", + `lighthouse@${LIGHTHOUSE_VERSION}`, + `${baseUrl}${route}`, + "--output=json", + `--output-path=${output}`, + `--preset=${strategy === "desktop" ? "desktop" : "perf"}`, + "--only-categories=performance", + "--chrome-flags=--headless=new --no-sandbox --disable-dev-shm-usage", + "--max-wait-for-load=60000", + "--quiet", + ], + { + cwd: projectRoot, + env: { ...offlineEnv, ...(chromePath ? { CHROME_PATH: chromePath } : {}) }, + stdio: "inherit", + }, + ); + + const readIfPresent = (file) => (existsSync(file) ? readFileSync(file, "utf8") : null); for (const strategy of strategies) { for (const route of routes) { + const cell = `${strategy} ${route}`; const output = path.join(reportDirectory, `${strategy}-${slugFor(route)}.json`); - console.log(`Measuring ${strategy} ${route}`); - const result = spawnSync( - npxInvocation.command, - [ - ...npxInvocation.prefixArgs, - "--yes", - `lighthouse@${LIGHTHOUSE_VERSION}`, - `${baseUrl}${route}`, - "--output=json", - `--output-path=${output}`, - `--preset=${strategy === "desktop" ? "desktop" : "perf"}`, - "--only-categories=performance", - "--chrome-flags=--headless=new --no-sandbox --disable-dev-shm-usage", - "--max-wait-for-load=60000", - "--quiet", - ], - { - cwd: projectRoot, - env: { ...offlineEnv, ...(chromePath ? { CHROME_PATH: chromePath } : {}) }, - stdio: "inherit", - }, - ); - // Not downgraded to a warning: the grader treats a missing report as - // incomplete evidence and fails, which is the behaviour we want locally too. - if (childProcessExitCode(result) !== 0) { - const failure = `${strategy} ${route}`; - failures.push(failure); - console.log(`::warning::lighthouse ${failure} failed (${childProcessFailureSummary(result)})`); + console.log(`Measuring ${cell}`); + let result = measure(strategy, route, output); + let reason = measurementFailureReason(childProcessExitCode(result), readIfPresent(output)); + + if (reason) { + // ONE retry, and it is announced. A cell that never started measured nothing + // about this diff, so treating it as a regression is wrong — but so is + // treating it as a pass, which is why nothing below is downgraded. If the + // retry also produces no measurement the report stays missing or errored and + // the grader's incompleteBudgetEvidence still fails closed regardless of + // `enforce`. The retry is reported either way, so a chronically flaky route + // cannot hide behind a green run. + console.log(`::warning::lighthouse ${cell} produced no measurement (${reason}); retrying once`); + // Never leave the first attempt's file behind: a runtimeError report would + // otherwise be graded, or baked into a refreshed baseline, if the retry fails + // before writing. + rmSync(output, { force: true }); + result = measure(strategy, route, output); + const after = measurementFailureReason(childProcessExitCode(result), readIfPresent(output)); + retried.push(`${cell} (${reason}${after ? ` -> still ${after}` : " -> recovered"})`); + reason = after; + } + + if (reason) { + failures.push(cell); + console.log( + `::warning::lighthouse ${cell} failed after one retry (${reason}; ${childProcessFailureSummary(result)})`, + ); } } } + // Emitted whether or not the retry recovered: a retry is a fact about the evidence + // and belongs in the run summary and the artifact, not only in the scrollback. + if (retried.length > 0) { + const line = `lighthouse retried ${retried.length} cell(s): ${retried.join("; ")}`; + console.log(`::warning::${line}`); + if (process.env.GITHUB_STEP_SUMMARY) { + appendFileSync(process.env.GITHUB_STEP_SUMMARY, `\n> Lighthouse ${line}\n`); + } + // Deliberately .txt, not .json: `readReports` in check-lighthouse-budget.mjs + // globs *.json and skips only summary.json, so a JSON sidecar here would be + // parsed as a Lighthouse report and become a phantom row named `retries`. + writeFileSync(path.join(reportDirectory, "retries.txt"), `${retried.join("\n")}\n`, "utf8"); + } + if (failures.length > 0) console.log(`::warning::lighthouse failed for ${failures.join(", ")}`); // --require-reports: this runner OWNS the directory and has just tried to measure diff --git a/tests/check-lighthouse-budget.test.ts b/tests/check-lighthouse-budget.test.ts index 9f6e1d719b..ff210308e5 100644 --- a/tests/check-lighthouse-budget.test.ts +++ b/tests/check-lighthouse-budget.test.ts @@ -1,4 +1,5 @@ -import { readFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; @@ -10,8 +11,10 @@ import { expectedBudgetRuns, gradeRun, incompleteBudgetEvidence, + readReports, renderBudgetTable, } from "../scripts/check-lighthouse-budget.mjs"; +import { measurementFailureReason } from "../scripts/lighthouse-measurement-outcome.mjs"; /** Kept in step with lighthouse-budget.json. */ const ROUTES = ["/", "/therapy-compass", "/documents/search", "/dsm", "/forms"]; @@ -133,13 +136,60 @@ describe("incompleteBudgetEvidence — completeness derived from what is graded" expect(result.reason).toBe("evidence incomplete"); }); - it("rejects a baseline measured by a different browser", () => { + it("rejects a baseline measured by a different browser, collapsed to one instruction", () => { + // One browser bump reds every run in the budget. Ten near-identical sentences + // buried the single actionable line, so drift collapses to one message when it is + // the whole story — the VERDICT is unchanged and asserted below. const rows = completeRows(); const stale = baselineFromRows(rows.map((entry: Row) => ({ ...entry, chromeVersion: "HeadlessChrome/131" }))); const problems = incompleteBudgetEvidence(rows, budget({ baseline: stale })); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("browser drift on 10 run(s)"); + expect(problems[0]).toContain("HeadlessChrome/131"); + expect(problems[0]).toContain("HeadlessChrome/140"); + expect(problems[0]).toContain("Refresh Lighthouse baseline"); + + // The collapse is cosmetic. Incomplete evidence still fails closed, and still + // does so independently of `enforce`. + for (const enforce of [true, false]) { + const result = compareToLighthouseBudget(rows, budget({ baseline: stale, enforce })); + expect(result.status).toBe("fail"); + expect(result.reason).toBe("evidence incomplete"); + } + }); + + it("lists drift per run when the browsers themselves disagree", () => { + // Two different baseline browsers is not one fact, so it must not read as one. + const rows = completeRows(); + const mixed = Object.fromEntries( + Object.entries(baselineFromRows(rows)).map(([run, entry], index) => [ + run, + { ...(entry as object), chromeVersion: index % 2 === 0 ? "HeadlessChrome/131" : "HeadlessChrome/132" }, + ]), + ); + const problems = incompleteBudgetEvidence(rows, budget({ baseline: mixed })); + expect(problems).toHaveLength(10); - expect(problems[0]).toContain("measured by a different browser"); + expect(problems.every((problem: string) => problem.includes("measured by a different browser"))).toBe(true); + }); + + it("keeps drift per run when a measurement gap shares the verdict", () => { + // A missing report and a browser bump are different facts with different fixes; + // collapsing here would hide the one that --update cannot resolve. + const rows = completeRows().filter((entry: Row) => entry.run !== "mobile-dsm"); + const stale = baselineFromRows( + completeRows().map((entry: Row) => ({ ...entry, chromeVersion: "HeadlessChrome/131" })), + ); + const problems = incompleteBudgetEvidence(rows, budget({ baseline: stale })); + + expect(problems).toEqual( + expect.arrayContaining([ + "mobile-dsm: no Lighthouse report produced", + expect.stringContaining("measured by a different browser"), + ]), + ); + expect(problems.length).toBeGreaterThan(1); }); it("ignores browser drift and missing baseline rows when refreshing", () => { @@ -343,6 +393,13 @@ describe("committed lighthouse-budget.json", () => { expect(committed.strategies).toEqual(["mobile", "desktop"]); }); + it("measures every route without a query string", () => { + // ci-change-scope.mjs excludes src/app/api/** from perf_changed on exactly this + // fact: a bare URL cannot issue an API request on load. A query-bearing route + // would silently invalidate that exclusion, so it is asserted in both places. + expect(committed.routes.filter((route) => route.includes("?"))).toEqual([]); + }); + it("invokes npm's JavaScript npx CLI through Node when available", () => { const runner = readFileSync(path.join(process.cwd(), "scripts", "run-lighthouse-budget.mjs"), "utf8"); @@ -355,6 +412,71 @@ describe("committed lighthouse-budget.json", () => { }); }); +describe("measurementFailureReason", () => { + const report = (extra: Record = {}) => + JSON.stringify({ requestedUrl: "http://localhost:4461/forms", audits: {}, ...extra }); + + it("passes a clean run through", () => { + expect(measurementFailureReason(0, report())).toBeNull(); + }); + + it("flags a non-zero exit", () => { + expect(measurementFailureReason(1, null)).toContain("exited 1"); + }); + + it("flags a run that was killed without a status", () => { + expect(measurementFailureReason(null, null)).toContain("without a status"); + }); + + it("flags a clean exit that wrote no report", () => { + expect(measurementFailureReason(0, null)).toBe("no report file was written"); + expect(measurementFailureReason(0, "")).toBe("no report file was written"); + }); + + it("flags an unparseable report", () => { + expect(measurementFailureReason(0, "{not json")).toBe("report is not valid JSON"); + }); + + it("flags the NO_NAVSTART shape that exits zero with a well-formed report", () => { + // Ledger #147: `/forms` did this locally while the live dispatch measured it + // fine. An exit-code check alone leaves it unretried, because Lighthouse both + // exits 0 and writes a valid file whose only content is the runtime error. + expect(measurementFailureReason(0, report({ runtimeError: { code: "NO_NAVSTART" } }))).toBe( + "lighthouse runtimeError NO_NAVSTART", + ); + }); + + it("never retries a real measurement that produced bad numbers", () => { + // The line that keeps this a retry and not a re-roll: a page that loaded and + // scored badly is evidence, and re-running it until it goes green is the failure + // mode this whole gate exists to prevent. + const slow = report({ audits: { "largest-contentful-paint": { numericValue: 9999 } } }); + + expect(measurementFailureReason(0, slow)).toBeNull(); + }); +}); + +describe("readReports", () => { + it("ignores the retry sidecar rather than reading it as a report", () => { + // retries.txt is deliberately not .json: readReports globs *.json and skips only + // summary.json, so a JSON sidecar would be parsed as a Lighthouse report and + // become a phantom row named `retries` — a run the budget never asked for, + // carrying no metrics. + const directory = mkdtempSync(path.join(tmpdir(), "lighthouse-reports-")); + try { + writeFileSync( + path.join(directory, "mobile-root.json"), + JSON.stringify({ requestedUrl: "http://localhost:4461/", finalUrl: "http://localhost:4461/", audits: {} }), + ); + writeFileSync(path.join(directory, "retries.txt"), "mobile /forms (lighthouse runtimeError NO_NAVSTART)\n"); + + expect(readReports(directory).map((entry: { run: string }) => entry.run)).toEqual(["mobile-root"]); + } finally { + rmSync(directory, { recursive: true, force: true }); + } + }); +}); + describe("renderBudgetTable", () => { it("subordinates every measurement when the evidence is incomplete", () => { const rows = completeRows().filter((entry: Row) => entry.run !== "mobile-forms"); diff --git a/tests/ci-cache-safety.test.ts b/tests/ci-cache-safety.test.ts index d1c4d074eb..2a43a2dca6 100644 --- a/tests/ci-cache-safety.test.ts +++ b/tests/ci-cache-safety.test.ts @@ -308,3 +308,85 @@ describe.skipIf(process.platform === "win32")("PR required aggregate — cancell expect(offenders).toEqual([]); }); }); + +describe("Lighthouse budget routing", () => { + /** The `lighthouse-budget:` block, up to the next top-level job key. */ + const lighthouseJob = /\n lighthouse-budget:\n([\s\S]*?)(?=\n [a-z][\w-]*:\n)/.exec(workflow)?.[1] ?? ""; + const refreshJob = /\n lighthouse-baseline-refresh:\n([\s\S]*?)(?=\n [a-z][\w-]*:\n)/.exec(workflow)?.[1] ?? ""; + + it("finds both Lighthouse jobs", () => { + // Fails closed on a rename rather than turning every assertion below into a + // vacuous match against an empty string. + expect(lighthouseJob, "lighthouse-budget job not found in ci.yml").not.toBe(""); + expect(refreshJob, "lighthouse-baseline-refresh job not found in ci.yml").not.toBe(""); + }); + + it("exports perf_changed from the change-scope job", () => { + expect(workflow).toContain("perf_changed: ${{ steps.scope.outputs.perf_changed }}"); + }); + + it("keys the budget off perf scope, not the old ui/build union", () => { + // `ui_changed || build_changed` put every dependabot lockfile bump and every + // worker/** change through a ~7 minute build plus ten Lighthouse runs. + expect(lighthouseJob).toContain("needs.changes.outputs.perf_changed == 'true'"); + expect(lighthouseJob).not.toContain("needs.changes.outputs.ui_changed"); + expect(lighthouseJob).not.toContain("needs.changes.outputs.build_changed"); + }); + + it("tests draft with `!= true`, so push and schedule runs survive", () => { + // `github.event.pull_request` is null on push/schedule/merge_group, so + // `draft == false` is FALSE there and would silently kill both arms. + expect(lighthouseJob).toContain("github.event.pull_request.draft != true"); + expect(lighthouseJob).not.toContain("github.event.pull_request.draft == false"); + }); + + it("reads the dispatch input through github.event.inputs, which is null off-dispatch", () => { + // The `inputs` context only exists for workflow_dispatch/workflow_call; the + // github.event.inputs form is a string and is safely null everywhere else. + expect(lighthouseJob).toContain("github.event.inputs.refresh_lighthouse_baseline != 'true'"); + expect(refreshJob).toContain("github.event.inputs.refresh_lighthouse_baseline == 'true'"); + expect(workflow).toMatch(/workflow_dispatch:\n\s+inputs:\n\s+refresh_lighthouse_baseline:/); + }); + + it("pairs promotion to pr-required with merge_group coverage", () => { + // The budget skips merge_group ONLY because it is advisory and outside + // pr-required, where it could add ~7 minutes of merge latency without ever + // changing the outcome. Promoting it (#118) without restoring merge_group would + // leave the queue running a required check the PR never re-verified. + const prRequiredNeeds = /\n pr-required:\n[\s\S]*?needs:\s*\n?\s*\[([\s\S]*?)\]/.exec(workflow)?.[1] ?? ""; + // Fail closed on a lost anchor: an empty match would silently make this guard + // conclude "not required" forever, which is the branch that checks the least. + expect(prRequiredNeeds, "could not read pr-required's needs list from ci.yml").not.toBe(""); + expect(prRequiredNeeds).toContain("static-pr"); + const isRequired = /\blighthouse-budget\b/.test(prRequiredNeeds); + + if (isRequired) { + expect(lighthouseJob, "lighthouse-budget is required — it must also run in merge_group").toContain("merge_group"); + expect(lighthouseJob, "a required check must not be continue-on-error").not.toContain("continue-on-error: true"); + } else { + expect(lighthouseJob).toContain("continue-on-error: true"); + } + }); + + it("keeps the baseline refresh dispatch-only, red on failure, and unable to push", () => { + // A workflow that can rewrite a gate's own baseline is a gate that can green + // itself, so this job only ever produces an artifact for a human to commit. + expect(refreshJob).toContain("github.event_name == 'workflow_dispatch'"); + expect(refreshJob).not.toContain("continue-on-error"); + expect(refreshJob).not.toContain("git push"); + expect(refreshJob).not.toContain("persist-credentials: true"); + // An empty artifact would look like a successful refresh that recorded nothing. + expect(refreshJob).toContain("if-no-files-found: error"); + expect(refreshJob).toContain("--update"); + }); + + it("pins Chromium through one shared action in both jobs", () => { + // If the measuring and refreshing jobs resolve different browsers, the refreshed + // baseline records a browser other than the one grading against it and the gate + // goes permanently red — the exact failure this action was extracted to end. + expect(lighthouseJob).toContain("uses: ./.github/actions/setup-lighthouse-chromium"); + expect(refreshJob).toContain("uses: ./.github/actions/setup-lighthouse-chromium"); + expect(lighthouseJob).not.toContain("playwright install"); + expect(refreshJob).not.toContain("playwright install"); + }); +}); From b3fd373645873b464aef3cd913670e7da27839ca Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 8 Aug 2026 08:53:32 +0000 Subject: [PATCH 2/2] fix(ci): close Lighthouse perf-scope holes from review Restore the documented push-to-main lockfile backstop, keep the request proxy and initial-load API handlers in perf_changed, and include the shared Chromium pin action so measure/refresh drift stays in scope. Co-authored-by: BigSimmo --- .github/workflows/ci.yml | 18 +++++-- docs/process-hardening.md | 14 ++--- docs/testing.md | 27 +++++----- scripts/ci-change-scope.mjs | 73 ++++++++++++++++++++------- tests/check-lighthouse-budget.test.ts | 7 +-- tests/ci-cache-safety.test.ts | 9 ++++ 6 files changed, 103 insertions(+), 45 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c64cce3353..77b3e8be4a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -730,10 +730,12 @@ jobs: # merge latency and can never change the outcome. If it is # ever promoted (#118), merge_group MUST come back — # tests/ci-cache-safety.test.ts enforces that pairing. - # push (main/…) – always on perf scope. This lane catches what the PR arm - # deliberately does not: a runtime dependency bump, which - # perf scope excludes because ci-change-scope.mjs sees paths, - # not lockfile contents. + # push (main/…) – perf scope, PLUS lockfile_changed. The lockfile arm is the + # backstop for a runtime dependency bump: perf scope excludes + # package.json / package-lock.json because ci-change-scope.mjs + # sees paths, not lockfile contents, so without this arm a + # lockfile-only push would skip Lighthouse on both the PR and + # the subsequent push to main and leave only the weekly schedule. # schedule – weekly; resolves to the full-run sentinel, which trips perf # scope (asserted in ci-change-scope.mjs's self-tests). # workflow_dispatch – always, except a run dispatched to refresh the baseline @@ -757,7 +759,13 @@ jobs: github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'lighthouse-budget') || ( - needs.changes.outputs.perf_changed == 'true' && + ( + needs.changes.outputs.perf_changed == 'true' || + ( + github.event_name == 'push' && + needs.changes.outputs.lockfile_changed == 'true' + ) + ) && github.event_name != 'merge_group' && github.event.pull_request.draft != true ) diff --git a/docs/process-hardening.md b/docs/process-hardening.md index 5857a5422a..4f71bdc1cc 100644 --- a/docs/process-hardening.md +++ b/docs/process-hardening.md @@ -313,12 +313,14 @@ Three gates added for the "mature repo" verification pass. Full usage is in baseline is a gate that can green itself. - **The budget runs on perf scope, not the ui/build union (2026-08-08).** `perf_changed` in `scripts/ci-change-scope.mjs` excludes `worker/**` and container surfaces, dependency manifests and - the lockfile, Playwright/test surfaces, `src/app/api/**` and `src/app/mockups/**`. The lockfile - exclusion is the one deliberate hole: the classifier sees paths, never the lockfile diff, so it - cannot tell a devDependency bump from a React/Next bump. It is covered by `check:bundle-budget` on - the same PRs and by the `push`-to-`main` arm of the job's `if:`. If a runtime dep bump ever lands an - unnoticed LCP regression, teach the classifier to read the lockfile diff — do not put the whole - lockfile back into perf scope. + the lockfile, Playwright/test surfaces, most of `src/app/api/**` (except the initial-load handlers + `/api/setup-status` and `/api/local-project-id`), and `src/app/mockups/**`. `src/proxy.ts` stays in + scope because it runs before every budgeted navigation. The lockfile exclusion is the one + deliberate hole on the PR arm: the classifier sees paths, never the lockfile diff, so it cannot + tell a devDependency bump from a React/Next bump. The push-to-`main` arm re-runs when + `lockfile_changed` is true, and `check:bundle-budget` still covers the same PRs. If a runtime dep + bump ever lands an unnoticed LCP regression despite that, teach the classifier to read the lockfile + diff — do not put the whole lockfile back into perf scope. - **Verification debt remaining.** (1) No pixel baselines are committed, so `visual-baseline` reports rather than gates until an operator adopts them from the CI artifact. (2) A Lighthouse baseline is committed, but `enforce` stays `false` until #147 (`/dsm` mobile CLS 0.363) and #117 are resolved — diff --git a/docs/testing.md b/docs/testing.md index 3f295b74d4..67058524c0 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -171,21 +171,24 @@ Keyed off `perf_changed` (`scripts/ci-change-scope.mjs`), which is deliberately `ui_changed || build_changed` union it replaced — that union put every dependabot lockfile bump and every `worker/**` ingestion change through a ~7 minute isolated production build plus ten Lighthouse runs. In scope: `src/**`, `data/**`, `public/**`, `next.config.ts`, `postcss.config.mjs`, -`tsconfig.json`, and the budget's own inputs. Excluded: `worker/**` and container surfaces, dependency -manifests and the lockfile, Playwright/test surfaces including committed screenshots, -`src/app/api/**`, `src/app/mockups/**`, and the server/edge runtime entry points. An unrecognised path -under a listed root stays **in** scope, so a future refactor over-triggers by one job rather than -silently dropping a render surface. +`tsconfig.json`, the Chromium pin composite action, and the budget's own inputs. Excluded: +`worker/**` and container surfaces, dependency manifests and the lockfile, Playwright/test surfaces +including committed screenshots, most of `src/app/api/**` (except the initial-load handlers +`/api/setup-status` and `/api/local-project-id` that `/` always fetches), `src/app/mockups/**`, and +server/edge runtime entry points other than `src/proxy.ts` (which runs before every budgeted +navigation). An unrecognised path under a listed root stays **in** scope, so a future refactor +over-triggers by one job rather than silently dropping a render surface. Event matrix: pull requests on perf scope when not a draft; `merge_group` skipped while the job is advisory (it could only add merge latency without changing the outcome — promoting it to `pr-required` must restore `merge_group` in the same edit, which `tests/ci-cache-safety.test.ts` -enforces); `push` to `main`/`release/**` always on perf scope, which is the lane that catches a -runtime dependency bump the PR arm deliberately excludes; the weekly `schedule`; and -`workflow_dispatch`. The `lighthouse-budget` label forces a run and `skip-lighthouse-budget` opts out, -with skip winning. Caveat: `on.pull_request.types` has no `labeled` (adding it would re-run all of CI -on every label change), so the opt-in label takes effect on the next push or re-run — use -`workflow_dispatch` for an immediate run. +enforces); `push` to `main`/`release/**` on perf scope **or** when `lockfile_changed` is true (the +lockfile arm is the backstop for a runtime dependency bump the PR arm deliberately excludes from +`perf_changed`); the weekly `schedule`; and `workflow_dispatch`. The `lighthouse-budget` label +forces a run and `skip-lighthouse-budget` opts out, with skip winning. Caveat: +`on.pull_request.types` has no `labeled` (adding it would re-run all of CI on every label change), +so the opt-in label takes effect on the next push or re-run — use `workflow_dispatch` for an +immediate run. This is distinct from `.github/workflows/live-web-vitals.yml`, which measures the deployed origin for ledger #017 and is dispatch-only — by the time it runs, `main` has already auto-deployed. Both pin @@ -204,7 +207,7 @@ UI scope runs a fail-fast `@critical` Chromium job on pull requests, then one re PR body synchronization is skipped unless the checked-out head actually contains `PR_POLICY_BODY.md`. The eval-canary liveness API probe runs once with the daily Ops Digest cadence rather than on every PR. These remove repeated provider-side work without weakening a required result. -Two further jobs are advisory (`continue-on-error`, deliberately outside `pr-required`): `visual-baseline` on UI scope and `lighthouse-budget` on the narrower perf scope (see "When the budget runs" above — `worker/**` and container surfaces, dependency manifests and the lockfile, Playwright/test surfaces, `src/app/api/**` and `src/app/mockups/**` are all excluded). Both upload their evidence on every run, pass or fail, because the artifact is the whole point on a first run — the baselines to adopt and the reports to grade. Promote either to required by adding it to `pr-required` and removing `continue-on-error` in the same edit; for `lighthouse-budget` that edit must also restore `merge_group` to its `if:`. +Two further jobs are advisory (`continue-on-error`, deliberately outside `pr-required`): `visual-baseline` on UI scope and `lighthouse-budget` on the narrower perf scope (see "When the budget runs" above — `worker/**` and container surfaces, dependency manifests and the lockfile, Playwright/test surfaces, most of `src/app/api/**` other than initial-load handlers, and `src/app/mockups/**` are excluded; `src/proxy.ts` stays in). Both upload their evidence on every run, pass or fail, because the artifact is the whole point on a first run — the baselines to adopt and the reports to grade. Promote either to required by adding it to `pr-required` and removing `continue-on-error` in the same edit; for `lighthouse-budget` that edit must also restore `merge_group` to its `if:`. ## Contribution checklist (UI changes) diff --git a/scripts/ci-change-scope.mjs b/scripts/ci-change-scope.mjs index 9ffe495097..5018332b21 100644 --- a/scripts/ci-change-scope.mjs +++ b/scripts/ci-change-scope.mjs @@ -206,17 +206,30 @@ const perfPatterns = [ // it matched no CI scope pattern at all. "lighthouse-budget.json", /^scripts\/(run-lighthouse-budget|check-lighthouse-budget|summarise-web-vitals|lighthouse-measurement-outcome)\.mjs$/, + // Measuring and refreshing jobs MUST resolve the same Chromium. Editing this action + // changes which browser grades the baseline, so it belongs in perf scope even when + // no application source moved. + ".github/actions/setup-lighthouse-chromium", ]; +/** + * App Router handlers that budgeted routes fetch during the initial Lighthouse + * navigation. ClinicalDashboard always calls /api/setup-status on mount, and + * readLocalProjectIdentity always calls /api/local-project-id before that. A bare + * URL with no query string does NOT imply "no API on load". + */ +const perfInitialLoadApiPatterns = ["src/app/api/setup-status", "src/app/api/local-project-id"]; + /** * Paths inside a `perfPatterns` root that cannot reach a measured route's render * path. Each is a deliberate loss of coverage; a path NOT listed here stays in scope. */ const perfExclusionPatterns = [ - // App Router API handlers. No measured route issues an API request during load: - // /documents/search is a static server-rendered shell, and every budget route is - // measured without a query string. assertBudgetRoutesAreQueryFree() fails closed if - // that ever changes. Precedent: isUiChangedPath already excludes API routes. + // Most App Router API handlers are off the Lighthouse critical path. Initial-load + // handlers in perfInitialLoadApiPatterns are carved out above and stay in scope. + // assertBudgetRoutesAreQueryFree() still fails closed if a budget route gains a + // query string — that is the usual signal that more handlers may need carving out. + // Precedent: isUiChangedPath already excludes API routes from the UI lane. "src/app/api", // Separate route segments, 404 in production, and the runner sets // NEXT_PUBLIC_MOCKUPS_ENABLED=false. Tailwind does scan src/**, so a utility class @@ -224,14 +237,16 @@ const perfExclusionPatterns = [ // an lcpMs floor of +100ms, and check:bundle-budget still enforces gzip growth. "src/app/mockups", // Server/edge runtime only. src/instrumentation-client.ts is deliberately NOT here: - // it executes in the browser and is a direct TBT contributor. + // it executes in the browser and is a direct TBT contributor. src/proxy.ts is also + // NOT here: its matcher runs before every budgeted page request (CSP/nonce and + // optional session refresh), so added latency there moves TTFB/LCP directly. "src/instrumentation.ts", - "src/proxy.ts", "src/sentry.server.config.ts", "src/sentry.edge.config.ts", ]; function isPerfChangedPath(filePath) { + if (pathMatches(filePath, perfInitialLoadApiPatterns)) return true; if (pathMatches(filePath, perfExclusionPatterns)) return false; return pathMatches(filePath, perfPatterns); } @@ -599,14 +614,12 @@ function assertMockupSpecParity() { } /** - * `perfExclusionPatterns` drops `src/app/api/**` on one measured fact: every route in - * `lighthouse-budget.json` is requested as a bare URL, so none of them issues an API - * request during load. A route gaining a query string (`/documents/search?q=…`) would - * silently invalidate that, and the resulting miss is exactly the kind that shows a - * green PR while the perf gate never ran against the code it depends on. - * - * So the assumption is guarded rather than hoped for. Fails CLOSED on an unreadable - * budget, like the mockup guard above. + * Budget routes stay query-free so a silent `?q=` addition cannot hide new + * client-driven API traffic from review. This is a signal, not a complete proof that + * no API runs on load — `/` already fetches /api/setup-status and + * /api/local-project-id via ClinicalDashboard, and those handlers are carved into + * perfInitialLoadApiPatterns. A new query-bearing route usually means more handlers + * need the same carve-out. Fails CLOSED on an unreadable budget. */ function assertBudgetRoutesAreQueryFree() { let budget; @@ -631,7 +644,7 @@ function assertBudgetRoutesAreQueryFree() { if (withQuery.length > 0) { throw new Error( `budget-routes-query-free: ${withQuery.join(", ")} carries a query string. A query-bearing budget route may ` + - "fetch src/app/api/** on load, so remove the `src/app/api` entry from perfExclusionPatterns or drop the route.", + "fetch additional src/app/api/** handlers on load — extend perfInitialLoadApiPatterns or drop the route.", ); } console.log(`Budget routes query-free: ${routes.length} routes carry no query string.`); @@ -791,9 +804,10 @@ function selfTest() { // #1668 (dependabot js-yaml, a devDependency) paid the full budget run. This // classifier only ever sees PATHS, never the lockfile diff, so it cannot tell a - // devDependency bump from a React/Next bump. The gap is covered by - // check:bundle-budget (enforced on the same PRs), by the push-to-main arm of the - // job's `if:`, and by the weekly schedule — NOT by putting the lockfile back here. + // devDependency bump from a React/Next bump. The PR arm therefore leaves + // perf_changed=false for manifests; the push-to-main arm of the job's `if:` + // re-runs when lockfile_changed is true, and the weekly schedule remains the + // delayed backstop. Do NOT put the lockfile back into perfPatterns. assertScope("perf-off-for-dependency-manifests", ["package.json", "package-lock.json"], { build_changed: true, lockfile_changed: true, @@ -813,6 +827,17 @@ function selfTest() { ui_changed: false, perf_changed: false, }); + // Initial-load handlers the budgeted `/` dashboard always fetches on mount. + assertScope("perf-on-for-initial-load-setup-status", ["src/app/api/setup-status/route.ts"], { + build_changed: true, + ui_changed: false, + perf_changed: true, + }); + assertScope("perf-on-for-initial-load-local-project-id", ["src/app/api/local-project-id/route.ts"], { + build_changed: true, + ui_changed: false, + perf_changed: true, + }); // A spec, fixture, golden PNG or browser config cannot change a byte the production // server sends, and the Lighthouse runner builds and serves its own isolated app // without ever loading Playwright. @@ -834,9 +859,15 @@ function selfTest() { }); assertScope( "perf-off-for-server-runtime-entrypoints", - ["src/instrumentation.ts", "src/proxy.ts", "src/sentry.server.config.ts", "src/sentry.edge.config.ts"], + ["src/instrumentation.ts", "src/sentry.server.config.ts", "src/sentry.edge.config.ts"], { build_changed: true, perf_changed: false }, ); + // The request proxy runs before every budgeted navigation (CSP/nonce, optional + // session refresh). Keeping it out of perf scope would miss TTFB/LCP regressions. + assertScope("perf-on-for-request-proxy", ["src/proxy.ts"], { + build_changed: true, + perf_changed: true, + }); assertScope("perf-off-for-bundle-budget-config", ["bundle-budget.json"], { build_changed: true, perf_changed: false, @@ -877,6 +908,10 @@ function selfTest() { assertScope("perf-on-for-retry-outcome-module", ["scripts/lighthouse-measurement-outcome.mjs"], { perf_changed: true, }); + assertScope("perf-on-for-chromium-pin-action", [".github/actions/setup-lighthouse-chromium/action.yml"], { + workflow_changed: true, + perf_changed: true, + }); // Proves the unknown-path direction: a new top-level directory under src/ is IN // scope, so a future refactor over-triggers by one job rather than silently // dropping a render surface. diff --git a/tests/check-lighthouse-budget.test.ts b/tests/check-lighthouse-budget.test.ts index ff210308e5..50579a75d4 100644 --- a/tests/check-lighthouse-budget.test.ts +++ b/tests/check-lighthouse-budget.test.ts @@ -394,9 +394,10 @@ describe("committed lighthouse-budget.json", () => { }); it("measures every route without a query string", () => { - // ci-change-scope.mjs excludes src/app/api/** from perf_changed on exactly this - // fact: a bare URL cannot issue an API request on load. A query-bearing route - // would silently invalidate that exclusion, so it is asserted in both places. + // Budget routes stay query-free so a silent `?q=` addition cannot hide new + // client-driven API traffic. This is a signal, not a complete proof that no API + // runs on load — `/` already fetches /api/setup-status and /api/local-project-id, + // and those handlers are carved into perfInitialLoadApiPatterns. expect(committed.routes.filter((route) => route.includes("?"))).toEqual([]); }); diff --git a/tests/ci-cache-safety.test.ts b/tests/ci-cache-safety.test.ts index 2a43a2dca6..444d39912d 100644 --- a/tests/ci-cache-safety.test.ts +++ b/tests/ci-cache-safety.test.ts @@ -333,6 +333,15 @@ describe("Lighthouse budget routing", () => { expect(lighthouseJob).not.toContain("needs.changes.outputs.build_changed"); }); + it("re-runs Lighthouse on push when the lockfile changed", () => { + // perf_changed deliberately stays false for package.json / package-lock.json + // (paths cannot distinguish a React bump from a js-yaml bump). Without this + // push arm, a lockfile-only merge would skip Lighthouse on the PR and again + // on the push to main, leaving only the weekly schedule. + expect(lighthouseJob).toContain("github.event_name == 'push'"); + expect(lighthouseJob).toContain("needs.changes.outputs.lockfile_changed == 'true'"); + }); + it("tests draft with `!= true`, so push and schedule runs survive", () => { // `github.event.pull_request` is null on push/schedule/merge_group, so // `draft == false` is FALSE there and would silently kill both arms.