diff --git a/.github/workflows/performance-budget.yml b/.github/workflows/performance-budget.yml index 0a5953bd1c..6b4f608a91 100644 --- a/.github/workflows/performance-budget.yml +++ b/.github/workflows/performance-budget.yml @@ -7,12 +7,80 @@ on: - 'packages/**' - 'apps/console/**' - 'pnpm-lock.yaml' + # This file itself (objectui#6245). Without it a change to this gate ships + # to `main` having never run once: the PR that edits it touches no + # `packages/**` path, so `Bundle Analysis` never appears on it, and a + # wiring bug surfaces on the NEXT `packages/**` PR — a required context + # turning red on someone else's diff, reading to them as a bundle problem + # of their own. Self-inclusion is this repo's convention for a + # path-filtered gate: measured on `origin/main`, 5 of the 7 workflows + # carrying a `paths:` filter list their own file, and this was one of the + # two that did not. The cost is honest and accepted — every edit of this + # file now pays a console build — and it is the same tax the other + # self-included gates already pay. + - '.github/workflows/performance-budget.yml' + # ...and the gate's RUNTIME CLOSURE: the files this job actually executes. + # Listing the YAML alone was still not enough — a PR touching only + # `check-eager-closure-budget.mjs`, the file that computes the verdict and + # the one this card is about, did not trigger this gate. The convention + # measured on `origin/main` is the closure, not just the workflow: + # `half-state-patrol.yml` lists its script AND `scripts/invoked-as.mjs`, + # the dependency of that script. + # + # `invoked-as.mjs` is here for the same reason it is there, and the reason + # is specific rather than tidy: `isEntrypoint` decides whether `main()` + # runs at all, so a regression in it makes this checker exit 0 having + # measured nothing — the exact silent-green failure the rest of this gate + # exists to prevent. + # + # NOT listed: `scripts/__tests__/*.test.ts`. This job never runs vitest — + # it runs two `node scripts/*.mjs` commands — so a test-only edit cannot + # change this gate's verdict, and those tests already run on every PR in + # the root vitest `unit` project. `published-dist-gate.yml` and + # `spec-range-floors.yml` draw the same line. + - 'scripts/check-eager-closure-budget.mjs' + - 'scripts/render-budget-comment.mjs' + - 'scripts/invoked-as.mjs' pull_request: branches: [main, develop] paths: - 'packages/**' - 'apps/console/**' - 'pnpm-lock.yaml' + # This file itself (objectui#6245). Without it a change to this gate ships + # to `main` having never run once: the PR that edits it touches no + # `packages/**` path, so `Bundle Analysis` never appears on it, and a + # wiring bug surfaces on the NEXT `packages/**` PR — a required context + # turning red on someone else's diff, reading to them as a bundle problem + # of their own. Self-inclusion is this repo's convention for a + # path-filtered gate: measured on `origin/main`, 5 of the 7 workflows + # carrying a `paths:` filter list their own file, and this was one of the + # two that did not. The cost is honest and accepted — every edit of this + # file now pays a console build — and it is the same tax the other + # self-included gates already pay. + - '.github/workflows/performance-budget.yml' + # ...and the gate's RUNTIME CLOSURE: the files this job actually executes. + # Listing the YAML alone was still not enough — a PR touching only + # `check-eager-closure-budget.mjs`, the file that computes the verdict and + # the one this card is about, did not trigger this gate. The convention + # measured on `origin/main` is the closure, not just the workflow: + # `half-state-patrol.yml` lists its script AND `scripts/invoked-as.mjs`, + # the dependency of that script. + # + # `invoked-as.mjs` is here for the same reason it is there, and the reason + # is specific rather than tidy: `isEntrypoint` decides whether `main()` + # runs at all, so a regression in it makes this checker exit 0 having + # measured nothing — the exact silent-green failure the rest of this gate + # exists to prevent. + # + # NOT listed: `scripts/__tests__/*.test.ts`. This job never runs vitest — + # it runs two `node scripts/*.mjs` commands — so a test-only edit cannot + # change this gate's verdict, and those tests already run on every PR in + # the root vitest `unit` project. `published-dist-gate.yml` and + # `spec-range-floors.yml` draw the same line. + - 'scripts/check-eager-closure-budget.mjs' + - 'scripts/render-budget-comment.mjs' + - 'scripts/invoked-as.mjs' concurrency: group: bundle-analysis-${{ github.event.pull_request.number || github.ref }} @@ -31,6 +99,12 @@ jobs: uses: actions/checkout@v7 with: submodules: true + # Depth 2, not 1: on a `pull_request` run HEAD is the MERGE REF, whose + # first parent is the base commit this checkout was computed against. + # The ceiling-freshness half (objectui#6245) needs that commit's copy + # of the checker to tell a stale checkout from a PR that re-baselines + # on purpose, and one extra commit is cheaper than a second fetch. + fetch-depth: 2 - name: Enable Corepack run: corepack enable @@ -62,6 +136,72 @@ jobs: - name: Build Console run: pnpm --filter @object-ui/console build + # objectui#6245: `Bundle Analysis` is a required context, and GitHub does + # not re-run a PR's checks when the base branch moves — so a green verdict + # can be computed against ceiling constants `main` has since replaced, and + # the merge is then gated on a ceiling that no longer exists. Observed + # live: run 32804357171 started at 03:13:27Z, 6m50s after `0409b766d` + # lowered MAX_EAGER_CLOSURE_GZIP_BYTES from 4,086,000 to 3,345,000, and + # published `BUDGET_CLOSURE_BUDGET_KB: 3990.2` — the retired ceiling — as a + # success. + # + # This step hands the checker THREE readings of the constants: the two + # below plus its own checkout. Two would not do — a re-baseline PR differs + # from the base branch deliberately and has to stay landable — so the + # verdict fires only when the base branch moved a ceiling AND this checkout + # does not carry the move. See `evaluateCeilingFreshness`. + # + # `continue-on-error` is deliberate and is NOT a fail-open: when this step + # cannot produce both files the checker finds the variables unset and + # reports a freshness ERROR (exit 2), which fails the job. Letting the step + # itself fail the job would kill it before the bundle is measured, and a + # run that hides its numbers is how this gate loses its readers + # (objectui#3152). + # + # The paths go out through `$GITHUB_ENV` rather than a step `env:` block: + # `render-budget-comment.test.ts` walks every `NAME: ${{ steps.* }}` line + # in this file and requires the COMMENT RENDERER to read it, which is the + # right rule for the comment's inputs and the wrong one for the checker's. + - name: Resolve the base-branch ceiling constants + id: base_ceilings + if: ${{ github.event_name == 'pull_request' }} + continue-on-error: true + env: + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + BASE_REF: ${{ github.base_ref }} + run: | + set -euo pipefail + CHECKER='scripts/check-eager-closure-budget.mjs' + OUT="${RUNNER_TEMP}/eager-closure-ceilings" + mkdir -p "$OUT" + + # The base commit the merge ref was computed against. `fetch-depth: 2` + # normally makes it a local object already; the fetch is the fallback + # for a merge ref GitHub recomputed against a different base. + if ! git cat-file -e "${PR_BASE_SHA}^{commit}" 2>/dev/null; then + git fetch --no-tags --depth=1 origin "$PR_BASE_SHA" + fi + git cat-file -p "${PR_BASE_SHA}:${CHECKER}" > "$OUT/pr-base.partial" + mv "$OUT/pr-base.partial" "$OUT/pr-base.mjs" + + # The base branch as it stands right now — what the merge would land on. + git fetch --no-tags --depth=1 origin "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" + BASE_SHA=$(git rev-parse "refs/remotes/origin/${BASE_REF}") + git cat-file -p "${BASE_SHA}:${CHECKER}" > "$OUT/base.partial" + mv "$OUT/base.partial" "$OUT/base.mjs" + + # Written only after BOTH `mv`s: a half-populated pair would let the + # checker compare against a file it thinks is the base branch. + { + echo "EAGER_CLOSURE_PR_BASE_SOURCE=$OUT/pr-base.mjs" + echo "EAGER_CLOSURE_BASE_SOURCE=$OUT/base.mjs" + echo "EAGER_CLOSURE_PR_BASE_SHA=$PR_BASE_SHA" + echo "EAGER_CLOSURE_BASE_SHA=$BASE_SHA" + echo "EAGER_CLOSURE_BASE_REF=$BASE_REF" + } >> "$GITHUB_ENV" + + echo "Ceilings resolved: ${PR_BASE_SHA} (this checkout's base) vs ${BASE_SHA} (${BASE_REF} now)" + - name: Check console performance budget id: budget run: | @@ -159,7 +299,9 @@ jobs: # cannot name, which is what objectui#6230 was (the per-chunk and # sensitivity halves both went unread). A fourth half added here must # be wired through too; `render-budget-comment.test.ts` fails if it is - # not. + # not. The fourth half — ceiling freshness, objectui#6245 — arrived + # through exactly that door: it publishes `closure_freshness_status`, + # and is wired into the comment step below. # # Exit codes are distinct on # purpose: 1 = over budget (a verdict about the bundle), 2 = no @@ -173,7 +315,7 @@ jobs: if [ "$CLOSURE_CODE" -eq 2 ]; then echo "budget_status=error" >> "$GITHUB_OUTPUT" - echo "budget_message=The entry chunk measured ${GZIP_KB} KB, but the eager-closure gauge produced no trustworthy measurement — see the step log. This is a broken gauge, not a passing budget." >> "$GITHUB_OUTPUT" + echo "budget_message=The entry chunk measured ${GZIP_KB} KB, but the eager-closure half of this gate returned no trustworthy VERDICT: the report could not be read, a ceiling has drifted out of range of the regression it must catch, or (objectui#6245) a ceiling was replaced on the base branch after this checkout was made. The step log says which. This is not a passing budget — and it is not a size regression either." >> "$GITHUB_OUTPUT" exit 1 fi @@ -258,6 +400,9 @@ jobs: # half did — the reader has to open the job log to find out. BUDGET_CLOSURE_CHUNK_STATUS: ${{ steps.budget.outputs.closure_chunk_status }} BUDGET_CLOSURE_HEADROOM_STATUS: ${{ steps.budget.outputs.closure_headroom_status }} + # The freshness half (objectui#6245). Empty on a run it does not apply + # to; the renderer filters an empty half out rather than rendering it. + BUDGET_CLOSURE_FRESHNESS_STATUS: ${{ steps.budget.outputs.closure_freshness_status }} BUDGET_STEP_OUTCOME: ${{ steps.budget.outcome }} BUILD_PACKAGES_OUTCOME: ${{ steps.build_packages.outcome }} run: node scripts/render-budget-comment.mjs > budget-comment.md diff --git a/scripts/__tests__/check-eager-closure-budget.test.ts b/scripts/__tests__/check-eager-closure-budget.test.ts index 06278ef6b5..9a07b5fc23 100644 --- a/scripts/__tests__/check-eager-closure-budget.test.ts +++ b/scripts/__tests__/check-eager-closure-budget.test.ts @@ -14,9 +14,12 @@ import { PER_CHUNK_GZIP_CEILINGS, REGRESSION_THIS_GATE_MUST_CATCH_BYTES, SUPPORTED_REPORT_VERSION, + VERDICT_CEILING_CONSTANTS, + evaluateCeilingFreshness, evaluateClosureBudget, evaluateHeadroomSensitivity, evaluatePerChunkBudgets, + extractCeilingDeclarations, main, measureChunksByName, renderTopChunks, @@ -26,6 +29,7 @@ import { const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); const workflowPath = path.join(repoRoot, '.github/workflows/performance-budget.yml'); const viteConfigPath = path.join(repoRoot, 'apps/console/vite.config.ts'); +const checkerPath = path.join(repoRoot, 'scripts/check-eager-closure-budget.mjs'); /** * A report shaped exactly like `emitEagerClosureReport`'s output, with the @@ -561,15 +565,31 @@ describe('renderTopChunks', () => { }); describe('main', () => { - function run(reportBody: unknown) { + /** + * Hermetic by construction: `main`'s environment is INJECTED here, never + * inherited from the process. + * + * objectui#6245 — `main(argv, env = process.env)` is the right PRODUCTION + * default, but a test that leans on it is measuring the machine it runs on. + * `GITHUB_EVENT_NAME=pull_request` is ambient inside GitHub Actions, so this + * helper omitting `env` silently switched the ceiling-freshness half ON in + * CI — where its two source paths are unset, so the half correctly returned + * `error` and `main` correctly returned 2, into four assertions written when + * 0 and 1 were the only outcomes it could produce. Every one of them passed + * locally, for the single reason that proves nothing: the variable happened + * not to be set. + * + * `GITHUB_EVENT_NAME` is absent from the injected object ON PURPOSE — that is + * what makes these cases exercise the non-pull_request path deterministically + * instead of by luck. Pass it through `env` to opt a case in. + */ + function run(reportBody: unknown, env: Record = {}) { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'closure-budget-')); const reportPath = path.join(dir, 'eager-closure.json'); const outputPath = path.join(dir, 'github-output'); if (reportBody !== undefined) fs.writeFileSync(reportPath, JSON.stringify(reportBody)); - const previous = process.env.GITHUB_OUTPUT; - process.env.GITHUB_OUTPUT = outputPath; try { - const code = main(['--report', reportPath]); + const code = main(['--report', reportPath], { GITHUB_OUTPUT: outputPath, ...env }); const outputs = Object.fromEntries( fs .readFileSync(outputPath, 'utf8') @@ -579,12 +599,34 @@ describe('main', () => { ); return { code, outputs }; } finally { - if (previous === undefined) delete process.env.GITHUB_OUTPUT; - else process.env.GITHUB_OUTPUT = previous; fs.rmSync(dir, { recursive: true, force: true }); } } + /** + * The guard for the defect above, and it has to be a TEST rather than a note + * in the helper: the failure is invisible on a developer machine and appears + * only inside Actions, which is the worst place to find it — on the next + * person's unrelated PR. Setting the variable here reproduces CI's ambient + * environment in-process, so if `run()` ever goes back to inheriting + * `process.env` this reds locally, immediately, on the change that caused it. + */ + it('is unaffected by an ambient GITHUB_EVENT_NAME, the way Actions sets it', () => { + const previous = process.env.GITHUB_EVENT_NAME; + process.env.GITHUB_EVENT_NAME = 'pull_request'; + try { + const { code, outputs } = run(budgeted()); + expect(code).toBe(0); + // The freshness half must stay dormant. This run injects no base-branch + // sources, so an ACTIVE half would correctly report `error` and exit 2 — + // precisely how the ambient variable turned four green assertions red. + expect(outputs.closure_freshness_status).toBe(''); + } finally { + if (previous === undefined) delete process.env.GITHUB_EVENT_NAME; + else process.env.GITHUB_EVENT_NAME = previous; + } + }); + // `budgeted()` rather than the bare `report()` fixture: since objectui#5490 // the checker weighs BOTH halves, and a report missing the budgeted chunks is // an error — which is the per-chunk half working, not a fixture detail. @@ -761,6 +803,317 @@ describe('main', () => { }); }); +/** + * objectui#6245 — the fourth half. `Bundle Analysis` is a required context, and + * GitHub does not re-run a PR's checks when the base branch moves, so a green + * verdict can be computed against ceilings `main` has since replaced. + * + * Not hypothetical: run 32804357171 started 6m50s after `0409b766d` lowered the + * aggregate ceiling from 4,086,000 to 3,345,000 and published + * `BUDGET_CLOSURE_BUDGET_KB: 3990.2` — 4,086,000 bytes — with conclusion + * `success`. `theRealIncident` below replays exactly that pair of numbers. + */ +describe('ceiling freshness (objectui#6245)', () => { + const checkerSource = fs.readFileSync(checkerPath, 'utf8'); + + /** + * A copy of the real checker with one ceiling moved, so these fixtures track + * a future re-baseline instead of pinning today's digits in a second place. + * + * The `expect` is the guard on the guard: a `String.replace` whose pattern + * matches nothing returns the subject unchanged and throws nothing, which + * would leave every case below comparing a file with itself and passing for + * the wrong reason. + */ + function withAggregateCeiling(bytes: number) { + const moved = checkerSource.replace( + /^export const MAX_EAGER_CLOSURE_GZIP_BYTES = .*;$/m, + `export const MAX_EAGER_CLOSURE_GZIP_BYTES = ${bytes};`, + ); + expect(moved, 'the fixture anchor no longer matches the real declaration').not.toBe( + checkerSource, + ); + return moved; + } + + const onPullRequest = (overrides: Record = {}) => + evaluateCeilingFreshness({ + eventName: 'pull_request', + headSource: checkerSource, + prBaseSource: checkerSource, + baseSource: checkerSource, + baseRef: 'main', + ...overrides, + }); + + describe('extractCeilingDeclarations', () => { + it('finds every constant a verdict is computed from, in the real file', () => { + const { declarations, missing } = extractCeilingDeclarations(checkerSource); + expect(missing).toEqual([]); + expect([...declarations.keys()]).toEqual([...VERDICT_CEILING_CONSTANTS]); + expect(declarations.get('MAX_EAGER_CLOSURE_GZIP_BYTES')).toBe( + String(MAX_EAGER_CLOSURE_GZIP_BYTES), + ); + }); + + it('reads an expression as text, so both sides of a comparison are read alike', () => { + // `89 * 1024` must not be evaluated: the base-branch side is a blob that + // cannot be imported, so an evaluated 91136 here would never match it. + expect(extractCeilingDeclarations(checkerSource).declarations.get( + 'REGRESSION_THIS_GATE_MUST_CATCH_BYTES', + )).toBe('89 * 1024'); + }); + + it('erases formatting that cannot move a ceiling', () => { + const a = "export const MAX_EAGER_CLOSURE_GZIP_BYTES = 3_345_000;"; + const b = "export const MAX_EAGER_CLOSURE_GZIP_BYTES =\n 3345000;"; + const read = (s: string) => + extractCeilingDeclarations(s, ['MAX_EAGER_CLOSURE_GZIP_BYTES']).declarations.get( + 'MAX_EAGER_CLOSURE_GZIP_BYTES', + ); + expect(read(a)).toBe(read(b)); + }); + + it('erases comments and trailing commas inside an object ceiling', () => { + const plain = "export const PER_CHUNK_GZIP_CEILINGS = Object.freeze({ a: 1, b: 2 });"; + const noisy = + 'export const PER_CHUNK_GZIP_CEILINGS = Object.freeze({\n' + + ' // objectui#5490 — why this one is here\n' + + ' a: 1,\n' + + ' b: 2,\n' + + '});'; + const read = (s: string) => + extractCeilingDeclarations(s, ['PER_CHUNK_GZIP_CEILINGS']).declarations.get( + 'PER_CHUNK_GZIP_CEILINGS', + ); + expect(read(noisy)).toBe(read(plain)); + }); + + it('reports a name it cannot find rather than skipping the comparison', () => { + const { declarations, missing } = extractCeilingDeclarations('export const OTHER = 1;'); + expect(missing).toEqual([...VERDICT_CEILING_CONSTANTS]); + expect(declarations.size).toBe(0); + }); + + it('does not mistake a nested `;` for the end of the declaration', () => { + const source = + 'export const PER_CHUNK_GZIP_CEILINGS = Object.freeze({ a: 1, b: 2 });\n' + + 'export const AFTER = 9;'; + expect( + extractCeilingDeclarations(source, ['PER_CHUNK_GZIP_CEILINGS']).declarations.get( + 'PER_CHUNK_GZIP_CEILINGS', + ), + ).toBe('Object.freeze({ a: 1, b: 2 })'); + }); + }); + + describe('evaluateCeilingFreshness', () => { + it('is not applicable off a pull_request — the checkout IS the branch', () => { + for (const eventName of ['push', 'workflow_dispatch', undefined]) { + const verdict = evaluateCeilingFreshness({ eventName, headSource: checkerSource }); + expect(verdict.status).toBe('not-applicable'); + expect(verdict.superseded).toEqual([]); + } + }); + + it('passes when the base branch has not moved a ceiling since this checkout', () => { + const verdict = onPullRequest(); + expect(verdict.status).toBe('pass'); + expect(verdict.moved).toEqual([]); + }); + + /** + * The case that makes this a three-reading check and not a two-reading one. + * A re-baseline PR differs from the base branch DELIBERATELY, and failing it + * would make the one PR that must land unlandable. + */ + it('passes a PR that re-baselines a ceiling itself', () => { + const verdict = onPullRequest({ headSource: withAggregateCeiling(3_000_000) }); + expect(verdict.status).toBe('pass'); + expect(verdict.moved).toEqual([]); + expect(verdict.superseded).toEqual([]); + }); + + it('passes when the base branch moved a ceiling and this checkout carries it', () => { + const moved = withAggregateCeiling(3_000_000); + const verdict = onPullRequest({ headSource: moved, baseSource: moved }); + expect(verdict.status).toBe('pass'); + expect(verdict.moved).toEqual(['MAX_EAGER_CLOSURE_GZIP_BYTES']); + expect(verdict.superseded).toEqual([]); + expect(verdict.message).toContain('already carries the new value'); + }); + + it('ERRORS when the base branch replaced a ceiling this run weighed against', () => { + const verdict = onPullRequest({ baseSource: withAggregateCeiling(3_000_000) }); + expect(verdict.status).toBe('error'); + expect(verdict.superseded).toEqual(['MAX_EAGER_CLOSURE_GZIP_BYTES']); + expect(verdict.message).toContain(String(MAX_EAGER_CLOSURE_GZIP_BYTES)); + expect(verdict.message).toContain('3000000'); + }); + + /** + * The live incident, with its own numbers. #6229 moved the aggregate ceiling + * from 4,086,000 to 3,345,000 at 03:06:37Z; a PR run seven minutes later was + * still weighing against 4,086,000 and reported success. + */ + it('catches the incident this card was filed for', () => { + const theRealIncident = onPullRequest({ + headSource: withAggregateCeiling(4_086_000), + prBaseSource: withAggregateCeiling(4_086_000), + baseSource: withAggregateCeiling(3_345_000), + prBaseSha: '48e53814e', + baseSha: '0409b766d', + }); + expect(theRealIncident.status).toBe('error'); + expect(theRealIncident.message).toContain('weighed here : 4086000'); + expect(theRealIncident.message).toContain('in force now : 3345000'); + expect(theRealIncident.message).toContain('48e53814e -> 0409b766d'); + }); + + /** + * The whole point of routing this to exit 2 rather than exit 1. A reader who + * takes a freshness failure for a size failure goes hunting a regression + * that is not there — and the fix they reach for is widening the ceiling, + * which is the one thing this must never teach. + */ + it('reads as a superseded ceiling and NOT as a bundle that grew', () => { + const { message } = onPullRequest({ baseSource: withAggregateCeiling(3_000_000) }); + expect(message).toContain('NOTHING GREW'); + expect(message).toContain('not a size regression'); + expect(message).toContain('Do NOT widen a ceiling'); + expect(message).toMatch(/update this branch/i); + // The size half's vocabulary must not appear here. + expect(message).not.toMatch(/over the .* budget|over budget|BUDGET EXCEEDED/); + }); + + it('ERRORS rather than passing when the base branch could not be read', () => { + for (const missing of ['prBaseSource', 'baseSource'] as const) { + const verdict = onPullRequest({ [missing]: null }); + expect(verdict.status).toBe('error'); + expect(verdict.message).toContain('NOT that the ceilings agree'); + } + }); + + it('ERRORS rather than passing when a ceiling declaration cannot be located', () => { + const verdict = onPullRequest({ baseSource: 'export const SOMETHING_ELSE = 1;' }); + expect(verdict.status).toBe('error'); + expect(verdict.message).toContain('could not be located'); + expect(verdict.message).toContain('MAX_EAGER_CLOSURE_GZIP_BYTES'); + }); + }); + + /** + * Through `main`, because the exit code and the published verdict are what the + * workflow acts on — and because these two assertions hold against the whole + * pipeline rather than one exported function. + */ + describe('main folds freshness into the exit code', () => { + /** + * A within-budget report carrying the whole closure, not just the budgeted + * chunks — a report holding only those is an ERROR (objectui#5924), so a + * thinner fixture would exit 2 for the wrong reason and prove nothing about + * freshness. Local rather than shared: `describe('main')` has its own copy + * and reaching across describes to borrow it would couple these two blocks. + */ + function healthyReport() { + const named = [ + { fileName: 'assets/index-A.js', name: 'index', bytes: 0, gzipBytes: 25_910 }, + ...Object.entries(PER_CHUNK_BASELINE).map(([name, gzipBytes]) => ({ + fileName: `assets/${name}-hash.js`, + name, + bytes: 0, + gzipBytes, + })), + ]; + const namedTotal = named.reduce((n, f) => n + f.gzipBytes, 0); + const files = [ + ...named, + { + fileName: 'assets/rest-of-closure.js', + name: 'rest-of-closure', + bytes: 0, + gzipBytes: BASELINE.gzipBytes - namedTotal, + }, + ]; + return report({ + files, + eagerChunkCount: files.length, + eagerGzipBytes: files.reduce((n, f) => n + f.gzipBytes, 0), + eagerRawBytes: 0, + }); + } + + function runWithEnv({ + eventName, + prBase, + base, + }: { + eventName: string; + prBase?: string; + base?: string; + }) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'closure-freshness-')); + const reportPath = path.join(dir, 'eager-closure.json'); + const outputPath = path.join(dir, 'github-output'); + fs.writeFileSync(reportPath, JSON.stringify(healthyReport())); + const write = (name: string, body: string) => { + const at = path.join(dir, name); + fs.writeFileSync(at, body); + return at; + }; + try { + const code = main(['--report', reportPath], { + GITHUB_OUTPUT: outputPath, + GITHUB_EVENT_NAME: eventName, + EAGER_CLOSURE_PR_BASE_SOURCE: prBase === undefined ? undefined : write('pr-base.mjs', prBase), + EAGER_CLOSURE_BASE_SOURCE: base === undefined ? undefined : write('base.mjs', base), + EAGER_CLOSURE_BASE_REF: 'main', + }); + const outputs = Object.fromEntries( + fs + .readFileSync(outputPath, 'utf8') + .split('\n') + .filter(Boolean) + .map((line) => { + const at = line.indexOf('='); + return [line.slice(0, at), line.slice(at + 1)] as [string, string]; + }), + ); + return { code, outputs }; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + } + + it('exits 2 and publishes `error` when the ceiling it used was superseded', () => { + const { code, outputs } = runWithEnv({ + eventName: 'pull_request', + prBase: checkerSource, + base: withAggregateCeiling(3_000_000), + }); + // 2, not 1: the bundle is under budget and nothing grew. + expect(code).toBe(2); + expect(outputs.closure_freshness_status).toBe('error'); + expect(outputs.closure_status).toBe('pass'); + expect(outputs.closure_headroom_status).toBe('pass'); + }); + + it('exits 2 on a pull_request run whose base branch was never resolved', () => { + const { code, outputs } = runWithEnv({ eventName: 'pull_request' }); + expect(code).toBe(2); + expect(outputs.closure_freshness_status).toBe('error'); + }); + + it('publishes an EMPTY freshness verdict off a pull_request, never a pass', () => { + // An absent half is filtered out of the PR comment; a `pass` would assert + // a comparison that never happened. + const { code, outputs } = runWithEnv({ eventName: 'push' }); + expect(code).toBe(0); + expect(outputs.closure_freshness_status).toBe(''); + }); + }); +}); + /** * The checker can only be correct if the workflow keeps feeding it and the * build keeps emitting the report. Neither half is reachable from a unit test. diff --git a/scripts/__tests__/render-budget-comment.test.ts b/scripts/__tests__/render-budget-comment.test.ts index 76a79c7f7a..8588fb19db 100644 --- a/scripts/__tests__/render-budget-comment.test.ts +++ b/scripts/__tests__/render-budget-comment.test.ts @@ -311,6 +311,66 @@ describe('per-half closure verdicts', () => { expect(body).not.toContain('Which half objected'); }); + /** + * objectui#6245 — the fourth half. Also exit 2, also the not-measured branch, + * and it must NOT borrow the drifted-ceiling wording above: the gauge is fine + * here and the bundle is fine; the ceiling they were weighed against has been + * replaced on the base branch. A reader who takes "broken gauge" at face value + * goes hunting a report that cannot be read, and one who takes it for a size + * failure widens a ceiling — the one repair this must never suggest. + */ + it('names a superseded ceiling as its own verdict, not as a broken gauge', () => { + const { kind, body } = renderBudgetComment({ + status: 'error', + ...halves, + closureFreshnessStatus: 'error', + message: 'a ceiling was replaced on the base branch after this checkout was made', + budgetOutcome: 'failure', + buildOutcome: 'success', + }); + + expect(kind).toBe('not-measured'); + expect(body).toContain('| Ceiling freshness (checkout vs. base branch) | ⚠️ superseded ceiling |'); + expect(body).toContain('neither a size regression nor a drifted gauge'); + expect(body).toContain('Nothing grew'); + expect(body).toContain('do not widen a ceiling to clear it'); + // The OTHER exit-2 note is about a different thing and must stay away. + expect(body).not.toContain('a verdict about the ceiling, not about the bundle'); + expect(body).not.toContain('❌'); + expect(body).not.toContain('FAIL'); + }); + + it('keeps the two exit-2 notes separate when both halves object', () => { + const { body } = renderBudgetComment({ + status: 'error', + ...halves, + closureHeadroomStatus: 'error', + closureFreshnessStatus: 'error', + budgetOutcome: 'failure', + buildOutcome: 'success', + }); + + expect(body).toContain('| Ceiling sensitivity (headroom) | ⚠️ broken gauge |'); + expect(body).toContain('| Ceiling freshness (checkout vs. base branch) | ⚠️ superseded ceiling |'); + expect(body).toContain('a verdict about the ceiling, not about the bundle'); + expect(body).toContain('neither a size regression nor a drifted gauge'); + }); + + it('renders no freshness row on a run the half does not apply to', () => { + // The checker publishes an EMPTY freshness verdict off a pull_request. An + // empty half is silence, and a green comment must stay byte-for-byte what + // it was before this half existed. + const { body } = renderBudgetComment({ + status: 'fail', + ...halves, + closureChunkStatus: 'fail', + closureFreshnessStatus: '', + }); + + expect(body).toContain('| Per-chunk ceilings | ❌ over its ceiling |'); + expect(body).not.toContain('Ceiling freshness'); + }); + it('renders no half table when no half status was handed over', () => { // The objectui#3152 failure mode, in its per-half form: blanks must render // as silence, never be inferred into verdicts. @@ -320,6 +380,7 @@ describe('per-half closure verdicts', () => { closureStatus: '', closureChunkStatus: '', closureHeadroomStatus: '', + closureFreshnessStatus: '', }); expect(body).not.toContain('Which half objected'); @@ -439,6 +500,7 @@ describe('performance-budget.yml contract', () => { expect(published).toContain('closure_status'); expect(published).toContain('closure_chunk_status'); expect(published).toContain('closure_headroom_status'); + expect(published).toContain('closure_freshness_status'); for (const key of published) { expect(workflow, `workflow must pass steps.budget.outputs.${key} to the comment step`) @@ -446,6 +508,113 @@ describe('performance-budget.yml contract', () => { } }); + /** + * objectui#6245. The freshness half's inputs do NOT travel as `steps.*` env — + * they are the checker's, not the comment's — so the env-contract test above + * cannot see them, and without these three the wiring could rot silently while + * every other test in this file stayed green. The half would then report + * `error` on every PR, or, if `GITHUB_EVENT_NAME` stopped being the + * discriminator, quietly stop asking. + */ + describe('ceiling freshness inputs', () => { + const checker = fs.readFileSync(checkerPath, 'utf8'); + + it('fetches deep enough to see the base commit the merge ref was built on', () => { + expect(workflow).toContain('fetch-depth: 2'); + }); + + it('resolves both base-branch readings on pull_request runs only', () => { + expect(workflow).toContain('- name: Resolve the base-branch ceiling constants'); + expect(workflow).toContain("if: ${{ github.event_name == 'pull_request' }}"); + }); + + it('exports exactly the variables the checker reads', () => { + for (const name of [ + 'EAGER_CLOSURE_PR_BASE_SOURCE', + 'EAGER_CLOSURE_BASE_SOURCE', + 'EAGER_CLOSURE_PR_BASE_SHA', + 'EAGER_CLOSURE_BASE_SHA', + 'EAGER_CLOSURE_BASE_REF', + ]) { + expect(workflow, `workflow must export ${name}`).toContain(`echo "${name}=`); + expect(checker, `checker must read env.${name}`).toContain(`env.${name}`); + } + }); + + /** + * objectui#6245, second and third pass. The freshness half shipped a + * workflow half that `Bundle Analysis` could never run on: the PR editing it + * touched no `packages/**` path, so the gate never appeared on its own PR. + * Adding the YAML to the filters fixed that — and was still not enough. A PR + * touching only `check-eager-closure-budget.mjs`, the file that computes the + * verdict and the one this card is about, ALSO did not trigger the gate. + * + * The convention measured on `origin/main` is the gate's RUNTIME CLOSURE, + * not merely its own YAML: `half-state-patrol.yml` lists both the script it + * runs and `scripts/invoked-as.mjs`, that script's dependency. + * + * A wiring bug in any of these is fail-CLOSED — freshness `error`, exit 2 — + * so an untriggered gate does not fail quietly; it turns a REQUIRED context + * red on the next `packages/**` PR, belonging to another seat, reading to + * them as a bundle problem in their own diff. + */ + const TRIGGER_CLOSURE = [ + '.github/workflows/performance-budget.yml', + 'scripts/check-eager-closure-budget.mjs', + 'scripts/render-budget-comment.mjs', + // `isEntrypoint` decides whether `main()` runs at all — a regression here + // makes the checker exit 0 having measured nothing. + 'scripts/invoked-as.mjs', + ]; + + it('triggers on its whole runtime closure, so no part of this gate ships unexercised', () => { + // `#` lines are part of the block — an entry that needs explaining carries + // its reason inline — and `[ \t]*` rather than `\s*` so the run cannot walk + // past the end of the list into the next key. + const filters = [...workflow.matchAll(/^[ \t]*paths:\n((?:[ \t]*(?:- |#).*\n)+)/gm)].map( + (m) => m[1], + ); + // Guard the guard: `push` and `pull_request` both carry one, and they must + // not drift apart — a gate that runs on a PR but not on the merge (or the + // reverse) is worse than one that runs on neither. + expect(filters).toHaveLength(2); + for (const filter of filters) { + for (const entry of TRIGGER_CLOSURE) { + expect(filter, `every paths: filter must list ${entry}`).toContain(entry); + } + } + }); + + /** + * The closure is defined by what the JOB EXECUTES, which is why the + * `__tests__` files are deliberately absent from it: this job runs two + * `node scripts/*.mjs` commands and never vitest, so a test-only edit cannot + * move this gate's verdict, and those tests already run on every PR in the + * root vitest `unit` project. This asserts the definition rather than the + * list, so the two stay in step. + */ + it('lists every script the job runs, and nothing it merely tests', () => { + const invoked = [...workflow.matchAll(/^\s*(?:run: )?node (scripts\/[\w./-]+\.mjs)/gm)].map( + (m) => m[1], + ); + expect(invoked.length).toBeGreaterThan(0); + for (const script of new Set(invoked)) { + expect(TRIGGER_CLOSURE, `${script} is executed by this job, so it belongs in the closure`) + .toContain(script); + } + expect(TRIGGER_CLOSURE.some((e) => e.includes('__tests__'))).toBe(false); + }); + + it('lets the bundle be measured even when the resolve step fails', () => { + // The checker turns absent inputs into a freshness ERROR, so failing the + // job here instead would only cost the run its numbers (objectui#3152). + const step = workflow.slice(workflow.indexOf('- name: Resolve the base-branch ceiling')); + const budgetStep = step.indexOf('- name: Check console performance budget'); + expect(step.slice(0, budgetStep)).toContain('continue-on-error: true'); + expect(budgetStep).toBeGreaterThan(-1); + }); + }); + it('writes budget_status on every path the budget step can exit through', () => { // pass, fail, and the two "nothing to measure" errors. const statuses = [...workflow.matchAll(/budget_status=(\w+)/g)].map((m) => m[1]); diff --git a/scripts/check-eager-closure-budget.mjs b/scripts/check-eager-closure-budget.mjs index 43704042ba..b6025aa56f 100644 --- a/scripts/check-eager-closure-budget.mjs +++ b/scripts/check-eager-closure-budget.mjs @@ -155,6 +155,27 @@ * fails loudly instead of passing by weighing nothing. See that constant's * comment for the reasoning and for how to move one. * + * ## Is the ceiling still the one in force? (objectui#6245) + * + * The three halves above all read this file's constants and take them as given. + * None of them can ask whether the constants THEMSELVES are current, and on a + * `pull_request` run that question has a wrong answer that is invisible from + * inside the checkout: `Bundle Analysis` is a required context, GitHub does not + * re-run a PR's checks when the base branch moves, so a green verdict can be + * computed against ceilings `main` has since replaced — and the merge is gated + * on it. Measured, not inferred: run 32804357171 started 6m50s AFTER + * `0409b766d` lowered the aggregate ceiling to 3,345,000 and published + * `BUDGET_CLOSURE_BUDGET_KB: 3990.2` — the retired 4,086,000 — as a success. + * + * {@link evaluateCeilingFreshness} is the fourth half and closes that window + * inside the tool: it compares {@link VERDICT_CEILING_CONSTANTS} across three + * readings of this file — this checkout, the base commit the checkout was made + * from, and the base branch tip — and calls a ceiling the base branch moved out + * from under this run an ERROR (exit 2). Three readings rather than two because + * a re-baseline PR differs from the base branch deliberately and must still + * land; see that function for the rule and for the residual window it cannot + * close. + * * ## Raising it * * Re-baselining is legitimate — it is how a ratchet advances — but it is a @@ -165,6 +186,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { fileURLToPath } from 'node:url'; import { isEntrypoint } from './invoked-as.mjs'; /** @@ -813,6 +835,319 @@ export function evaluateHeadroomSensitivity({ }; } +/** + * The constants a verdict from this file is COMPUTED FROM — the three the three + * evaluators above take as their defaults, and no others. + * + * {@link BASELINE} and {@link PER_CHUNK_BASELINE} are deliberately absent. They + * record the measurements the ceilings were derived from and are read by the + * unit test and by a human; no verdict depends on them. Listing them here would + * make a baseline-only or comment-only edit on the base branch read as a + * superseded ceiling, and a freshness check that cries wolf is one people learn + * to click past. + */ +export const VERDICT_CEILING_CONSTANTS = Object.freeze([ + 'MAX_EAGER_CLOSURE_GZIP_BYTES', + 'PER_CHUNK_GZIP_CEILINGS', + 'REGRESSION_THIS_GATE_MUST_CATCH_BYTES', +]); + +/** + * Read one `export const NAME = ...;` initializer out of module SOURCE TEXT, + * with comments dropped and formatting normalised. + * + * Text, not values, because the two sides of the freshness comparison are this + * file at two different commits and only one of them can be imported: the other + * is a blob from the base branch. Comparing `89 * 1024` against `91136` would + * be comparing an expression with an evaluation, so both sides are read the + * same way — through this function — and a difference means a difference. + * + * What normalisation deliberately erases, because none of it moves a ceiling: + * comments, line breaks and indentation, trailing commas, and the `_` numeric + * separators (`3_345_000` and `3345000` are the same number). What it keeps is + * everything else, so a changed digit is a changed ceiling. + * + * @param {string} source + * @param {number} from index just past the `=` + * @returns {string | null} the normalised initializer, or null if unterminated + */ +function readInitializer(source, from) { + let depth = 0; + const out = []; + let i = from; + while (i < source.length) { + const ch = source[i]; + const pair = source.slice(i, i + 2); + if (pair === '//') { + while (i < source.length && source[i] !== '\n') i += 1; + out.push(' '); + continue; + } + if (pair === '/*') { + const end = source.indexOf('*/', i + 2); + if (end === -1) return null; + i = end + 2; + out.push(' '); + continue; + } + if (ch === "'" || ch === '"' || ch === '`') { + const literal = readStringLiteral(source, i); + if (literal === null) return null; + out.push(literal.text); + i = literal.end; + continue; + } + if (ch === '(' || ch === '[' || ch === '{') depth += 1; + if (ch === ')' || ch === ']' || ch === '}') depth -= 1; + if (ch === ';' && depth === 0) return normaliseDeclaration(out.join('')); + out.push(ch); + i += 1; + } + return null; +} + +/** @param {string} source @param {number} start index of the opening quote */ +function readStringLiteral(source, start) { + const quote = source[start]; + let i = start + 1; + while (i < source.length) { + if (source[i] === '\\') { + i += 2; + continue; + } + if (source[i] === quote) return { text: source.slice(start, i + 1), end: i + 1 }; + i += 1; + } + return null; +} + +/** @param {string} text */ +function normaliseDeclaration(text) { + return text + .replace(/(\d)_(?=\d)/g, '$1') + .replace(/\s+/g, ' ') + .replace(/,(\s*[}\])])/g, '$1') + .trim(); +} + +/** + * Pull the {@link VERDICT_CEILING_CONSTANTS} declarations out of module source. + * + * A name this cannot find goes in `missing` and is an ERROR upstream, never a + * skipped comparison: "I could not read the ceiling" and "the ceiling agrees" + * are the two answers a freshness check must never confuse, and only one of + * them is safe to be quiet about. + * + * @param {string | null | undefined} source + * @param {readonly string[]} [names] + * @returns {{ declarations: Map, missing: string[] }} + */ +export function extractCeilingDeclarations(source, names = VERDICT_CEILING_CONSTANTS) { + const declarations = new Map(); + const missing = []; + const text = typeof source === 'string' ? source : ''; + for (const name of names) { + const anchor = new RegExp(`^export const ${name}\\s*=`, 'm').exec(text); + const initializer = anchor === null ? null : readInitializer(text, anchor.index + anchor[0].length); + if (initializer === null || initializer === '') { + missing.push(name); + continue; + } + declarations.set(name, initializer); + } + return { declarations, missing }; +} + +/** + * The FOURTH half: is the ceiling this run weighed against still the ceiling in + * force on the base branch? (objectui#6245) + * + * ## The race + * + * `Bundle Analysis` is a required context, and a `pull_request` run checks out + * the MERGE REF — a merge of the PR head with the base branch as GitHub last + * computed it. GitHub does not re-run a PR's checks when the base branch moves, + * so a green verdict can be computed against ceiling constants that `main` has + * since replaced, and the merge is then gated on a verdict about a ceiling that + * no longer exists. + * + * Observed live rather than reasoned about: run 32804357171 started at + * 03:13:27Z, six minutes and fifty seconds after `0409b766d` lowered + * {@link MAX_EAGER_CLOSURE_GZIP_BYTES} from 4,086,000 to 3,345,000 on `main`, + * and published `BUDGET_CLOSURE_BUDGET_KB: 3990.2` — 4,086,000 bytes, the + * retired ceiling — with `conclusion: success`. It did not bite: the payload was + * 3222.6 KB, under both numbers. With the aggregate headroom now 44.0 KB against + * an 89.0 KB regression class, the next one has half a regression of room to + * hide in. + * + * ## Why THREE readings and not two + * + * Comparing this checkout's ceilings against the base branch's is not enough: a + * re-baseline PR differs from the base branch ON PURPOSE, and failing it would + * make the one PR that must land unlandable. So the question is not "do they + * differ" but "did the BASE BRANCH move them out from under this checkout": + * + * - `prBaseSource` — the checker at the base commit this checkout was + * computed from. What the base branch said when the merge ref was made. + * - `baseSource` — the checker at the base branch tip right now. What the + * merge would actually land on. + * - `headSource` — the checker in this checkout. What the verdict used. + * + * A ceiling is SUPERSEDED when it moved between the first two AND this checkout + * does not carry the move. A re-baseline PR moves it in `headSource` only, the + * first two agree, and nothing fires. + * + * ## Why `error` (exit 2) and not `fail` + * + * `fail` is a verdict about the BUNDLE — it grew past a line. Nothing has grown + * here and no ceiling has drifted; the measurement above is sound and the + * ceilings on the base branch are sound. What is wrong is that they were never + * weighed against each other. That is a verdict about the GAUGE, which is what + * exit 2 means in this file, and the message says so in as many words — a + * freshness failure that reads like "the bundle grew" would send its reader + * hunting a regression that is not there. + * + * ## What this closes, and what it does not + * + * It closes the window this card recorded: a run that EXECUTES after the + * constants move, with a checkout that predates it. It cannot close the window + * where the run finishes BEFORE the constants move and the PR is merged after — + * no code of ours runs at that moment. That residual is what the `push`-on-main + * run detects after the fact, and closing it needs "require branches to be up to + * date before merging", a repository setting and a maintainer-floor decision + * this file has no business making. + * + * @param {object} input + * @param {string} [input.eventName] `GITHUB_EVENT_NAME` + * @param {string | null} [input.headSource] this checkout's copy of this file + * @param {string | null} [input.prBaseSource] this file at the checkout's base commit + * @param {string | null} [input.baseSource] this file at the base branch tip + * @param {string} [input.prBaseSha] + * @param {string} [input.baseSha] + * @param {string} [input.baseRef] + * @returns {{ status: 'pass' | 'error' | 'not-applicable', message: string, + * moved: string[], superseded: string[] }} + */ +export function evaluateCeilingFreshness({ + eventName, + headSource, + prBaseSource, + baseSource, + prBaseSha, + baseSha, + baseRef, +} = {}) { + const base = { moved: [], superseded: [] }; + const branch = baseRef ? `\`${baseRef}\`` : 'the base branch'; + + // A `push` run's checkout IS the branch being weighed, and a local run has no + // merge to gate. Neither can hold a superseded ceiling, so neither gets a + // verdict — this half exists for the merge ref and nothing else. + if (eventName !== 'pull_request') { + return { + ...base, + status: 'not-applicable', + message: + `Ceiling freshness: not applicable to a ${eventName ? `\`${eventName}\`` : 'local'} run. ` + + `This check compares a pull_request merge ref against the branch it would land on; a ` + + `checkout that IS that branch cannot be behind it.`, + }; + } + + const absent = [ + prBaseSource ? null : 'EAGER_CLOSURE_PR_BASE_SOURCE', + baseSource ? null : 'EAGER_CLOSURE_BASE_SOURCE', + ].filter((name) => name !== null); + if (absent.length > 0) { + return { + ...base, + status: 'error', + message: + `Ceiling freshness cannot be judged on this pull_request run: ${absent.join(' and ')} ` + + `named no readable file.\n` + + `Those are exported by the \`Resolve the base-branch ceiling constants\` step in ` + + `.github/workflows/performance-budget.yml; on a pull_request run their absence means ` + + `that step did not complete, NOT that the ceilings agree.\n` + + `An unreadable base branch is an ERROR and never a quiet pass: from inside this ` + + `checkout a superseded ceiling looks exactly like a current one.`, + }; + } + + const sides = /** @type {[string, string | null | undefined][]} */ ([ + ['this checkout', headSource], + ['the base commit this checkout was made from', prBaseSource], + [`the ${baseRef ? `\`${baseRef}\`` : 'base branch'} tip`, baseSource], + ]); + const read = sides.map(([label, source]) => ({ label, ...extractCeilingDeclarations(source) })); + const unreadable = read.filter((side) => side.missing.length > 0); + if (unreadable.length > 0) { + return { + ...base, + status: 'error', + message: + `Ceiling freshness cannot be judged — the ceiling declarations could not be located:\n` + + unreadable + .map((side) => ` - in ${side.label}: ${side.missing.map((n) => `\`${n}\``).join(', ')}`) + .join('\n') + + `\nBoth sides are read by \`extractCeilingDeclarations\`, which looks for ` + + `\`export const NAME = ... ;\` at the top level of this file. A name it cannot find has ` + + `been renamed, moved, or reshaped — re-point VERDICT_CEILING_CONSTANTS at the ceilings ` + + `the verdict is now computed from. It is an error rather than a skipped comparison for ` + + `the same reason an absent budgeted chunk is: a check that weighs nothing passes forever.`, + }; + } + + const [head, prBase, current] = read.map((side) => side.declarations); + const moved = [...VERDICT_CEILING_CONSTANTS].filter( + (name) => prBase.get(name) !== current.get(name), + ); + const superseded = moved.filter((name) => head.get(name) !== current.get(name)); + + if (superseded.length === 0) { + return { + ...base, + moved, + status: 'pass', + message: + moved.length === 0 + ? `Ceiling freshness: all ${VERDICT_CEILING_CONSTANTS.length} ceiling constants are ` + + `unchanged on ${branch} since this checkout's base${baseSha ? ` (${baseSha})` : ''}, ` + + `so the verdicts above were weighed against the ceilings actually in force.` + : `Ceiling freshness: ${moved.map((n) => `\`${n}\``).join(', ')} moved on ${branch} ` + + `since this checkout's base, and this checkout already carries the new ` + + `value${moved.length === 1 ? '' : 's'} — the verdicts above are current.`, + }; + } + + const table = superseded + .map( + (name) => + ` ❌ ${name}\n` + + ` weighed here : ${head.get(name)}\n` + + ` in force now : ${current.get(name)}`, + ) + .join('\n'); + + return { + moved, + superseded, + status: 'error', + message: + `${superseded.length} ceiling constant${superseded.length === 1 ? '' : 's'} this run ` + + `weighed against ${superseded.length === 1 ? 'has' : 'have'} been SUPERSEDED: ${branch} ` + + `moved ${superseded.length === 1 ? 'it' : 'them'} after this checkout was made` + + `${prBaseSha && baseSha ? ` (${prBaseSha} -> ${baseSha})` : ''}, and nothing re-ran the ` + + `verdicts above.\n${table}\n` + + `⛔ NOTHING GREW. This is not a size regression and it is not a drifted ceiling: the ` + + `measurement above is sound, and so are the ceilings on ${branch}. The only thing wrong is ` + + `that they were never weighed against each other, so a green tick here would gate the ` + + `merge on a ceiling that no longer exists (objectui#6245).\n` + + `Fix: update this branch onto ${branch} — merge or rebase — and let Bundle Analysis run ` + + `again. ⛔ Do NOT widen a ceiling to clear this; re-baselining is a separate, deliberate ` + + `act and this run has no evidence for one.`, + }; +} + /** * The biggest eager chunks, so a failure names suspects instead of a total. * @param {{ files?: { fileName: string, gzipBytes: number }[] }} report @@ -823,6 +1158,21 @@ export function renderTopChunks(report, limit = 12) { return files.map((f) => ` ${kb(f.gzipBytes).padStart(9)} KB ${f.fileName}`).join('\n'); } +/** + * Read a source file for {@link evaluateCeilingFreshness}, or null when the + * path is absent or unreadable. Null is never "the same as" — it routes to a + * loud ERROR upstream. + * @param {string | undefined} sourcePath + */ +export function readSource(sourcePath) { + if (!sourcePath) return null; + try { + return fs.readFileSync(sourcePath, 'utf8'); + } catch { + return null; + } +} + /** @param {string} reportPath */ export function readReport(reportPath) { try { @@ -841,21 +1191,25 @@ function writeGithubOutput(entries, outputPath = process.env.GITHUB_OUTPUT) { /** * Exit codes: `0` within budget, `1` over budget — the aggregate ceiling or any - * per-chunk ceiling — and `2` no trustworthy measurement (report missing, - * stale-shaped, internally inconsistent, missing a budgeted chunk, or governed - * by a ceiling that has drifted out of range of the regression it must catch). + * per-chunk ceiling — and `2` no trustworthy verdict (report missing, + * stale-shaped, internally inconsistent, missing a budgeted chunk, governed by + * a ceiling that has drifted out of range of the regression it must catch, or — + * objectui#6245 — weighed against a ceiling the base branch has since replaced). + * + * The last of those is the one exit 2 case with a PERFECTLY GOOD measurement + * behind it, so nothing downstream may word exit 2 as "nothing was measured". * * `2` covers the unbuilt tree, and deliberately so: with no * `apps/console/dist/eager-closure.json` this check reports a BROKEN GAUGE and * the workflow fails the step. It never prints a verdict about a bundle nobody * weighed, and it never exits 0 having measured nothing. * - * All three halves are evaluated and printed before any of them decides the + * All FOUR halves are evaluated and printed before any of them decides the * code: a run that reports the total and hides which chunk moved (or hides - * whether either line still means anything) teaches readers to ignore the half - * they cannot see. + * whether either line still means anything, or whether the line it used is the + * line in force) teaches readers to ignore the half they cannot see. */ -export function main(argv = process.argv.slice(2)) { +export function main(argv = process.argv.slice(2), env = process.env) { const flagIndex = argv.indexOf('--report'); const reportPath = flagIndex === -1 ? DEFAULT_REPORT_PATH : argv[flagIndex + 1]; const resolved = path.resolve(reportPath); @@ -863,6 +1217,19 @@ export function main(argv = process.argv.slice(2)) { const result = evaluateClosureBudget({ report, reportPath }); const perChunk = evaluatePerChunkBudgets({ report, reportPath }); const sensitivity = evaluateHeadroomSensitivity({ report, reportPath }); + // The fourth half asks about the CEILING rather than the payload, so its + // inputs are source texts and not the report: this file as checked out, + // and this file at two commits on the base branch. See + // {@link evaluateCeilingFreshness}. + const freshness = evaluateCeilingFreshness({ + eventName: env.GITHUB_EVENT_NAME, + headSource: readSource(fileURLToPath(import.meta.url)), + prBaseSource: readSource(env.EAGER_CLOSURE_PR_BASE_SOURCE), + baseSource: readSource(env.EAGER_CLOSURE_BASE_SOURCE), + prBaseSha: env.EAGER_CLOSURE_PR_BASE_SHA, + baseSha: env.EAGER_CLOSURE_BASE_SHA, + baseRef: env.EAGER_CLOSURE_BASE_REF, + }); if (result.status === 'pass') { console.log(`✅ ${result.message}`); @@ -879,6 +1246,16 @@ export function main(argv = process.argv.slice(2)) { } else { console.error(`❌ ${sensitivity.message}`); } + // `not-applicable` is printed rather than skipped. A half that says nothing + // is indistinguishable from a half that was switched off, and this file's + // whole argument is that silence must never read as a pass. + if (freshness.status === 'pass') { + console.log(`✅ ${freshness.message}`); + } else if (freshness.status === 'not-applicable') { + console.log(`ℹ️ ${freshness.message}`); + } else { + console.error(`❌ ${freshness.message}`); + } if (report?.files?.length) { console.log(''); console.log('Largest eagerly loaded chunks (gzipped):'); @@ -892,7 +1269,13 @@ export function main(argv = process.argv.slice(2)) { closure_chunks: result.chunkCount === null ? '' : String(result.chunkCount), closure_chunk_status: perChunk.status, closure_headroom_status: sensitivity.status, - }); + // Empty on a run this half does not apply to, so the PR comment's half + // table filters it out instead of rendering a blank verdict as a row. + closure_freshness_status: freshness.status === 'not-applicable' ? '' : freshness.status, + // `env`, not `process.env`: every other input this function reads comes from + // the injected environment, and a default that reaches around it would make + // the outputs untestable through the same seam as the verdicts. + }, env.GITHUB_OUTPUT); // Distinct codes so the workflow can tell "over budget" (a real verdict about // the bundle) from "the gauge produced nothing" (a verdict about the gauge). @@ -900,13 +1283,15 @@ export function main(argv = process.argv.slice(2)) { // regression, and a size regression reported as a broken report — each of // which teaches readers to ignore the other. // - // `error` outranks `fail` across ALL THREE halves for the same reason it does + // `error` outranks `fail` across ALL FOUR halves for the same reason it does // within one: a report that cannot be trusted — or a ceiling that no longer - // measures the thing it names — makes its own size verdict meaningless, - // whichever half noticed first. objectui#5490 established that ordering over - // two halves; objectui#5924 adds the third under the same rule rather than - // giving sensitivity a code of its own. - const statuses = [result.status, perChunk.status, sensitivity.status]; + // measures the thing it names, or that is no longer the ceiling in force — + // makes its own size verdict meaningless, whichever half noticed first. + // objectui#5490 established that ordering over two halves; objectui#5924 + // added the third and objectui#6245 the fourth, each under the same rule + // rather than taking a code of its own. `not-applicable` is inert in the + // fold: it is the absence of a question, not the answer `pass`. + const statuses = [result.status, perChunk.status, sensitivity.status, freshness.status]; if (statuses.includes('error')) return 2; return statuses.includes('fail') ? 1 : 0; } diff --git a/scripts/render-budget-comment.mjs b/scripts/render-budget-comment.mjs index 55c6ad720c..10a7717867 100644 --- a/scripts/render-budget-comment.mjs +++ b/scripts/render-budget-comment.mjs @@ -15,8 +15,10 @@ * * `pass` the console bundle was measured and is within budget * `fail` the console bundle was measured and is OVER budget - * `error` the budget step ran but the build produced nothing measurable - * (no `dist` directory, or no JS in it) + * `error` the budget step ran but produced no trustworthy verdict — nothing + * measurable (no `dist` directory, or no JS in it), a ceiling that has + * drifted out of range of the regression it must catch, or one the + * base branch replaced after this checkout (objectui#6245) * absent the budget step never ran at all — an earlier step failed, or the * run was cancelled * @@ -50,6 +52,7 @@ const text = (value) => (typeof value === 'string' ? value.trim() : ''); * @param {string} [input.closureChunks] how many chunks the eager closure spans * @param {string} [input.closureChunkStatus] `closure_chunk_status` — the per-chunk half * @param {string} [input.closureHeadroomStatus] `closure_headroom_status` — the sensitivity half + * @param {string} [input.closureFreshnessStatus] `closure_freshness_status` — the freshness half * @param {string} [input.message] human-readable reason when `status` is `error` * @param {string} [input.budgetOutcome] `steps.budget.outcome` * @param {string} [input.buildOutcome] `steps.build_packages.outcome` @@ -70,6 +73,7 @@ export function renderBudgetComment(input = {}) { chunks: text(input.closureChunks), chunkStatus: text(input.closureChunkStatus), headroomStatus: text(input.closureHeadroomStatus), + freshnessStatus: text(input.closureFreshnessStatus), }; // A verdict needs an affirmative status AND the numbers that status was @@ -93,17 +97,35 @@ export function renderBudgetComment(input = {}) { } /** - * The eager-closure checker evaluates three halves and publishes a verdict for - * each. The step's exit code folds all three into ONE `budget_status`, so a + * The eager-closure checker evaluates four halves and publishes a verdict for + * each. The step's exit code folds all four into ONE `budget_status`, so a * comment that renders only that says "something objected" and sends the reader - * to the job log to learn which — the aggregate total, one chunk, or a ceiling - * that has stopped measuring anything (objectui#6230). These labels name the - * halves the way the step log names them. + * to the job log to learn which — the aggregate total, one chunk, a ceiling that + * has stopped measuring anything (objectui#6230), or a ceiling the base branch + * replaced after this checkout (objectui#6245). These labels name the halves the + * way the step log names them. + * + * The optional third element overrides the verdict wording for one half. + * Freshness needs it: `⚠️ broken gauge` would be FALSE there — the gauge is + * fine, it was simply pointed at a retired number — and a reader who takes + * "broken gauge" at face value goes looking for a report that cannot be read. */ +const FRESHNESS_HALF = 'freshnessStatus'; + const CLOSURE_HALVES = [ ['status', 'Aggregate closure ceiling'], ['chunkStatus', 'Per-chunk ceilings'], ['headroomStatus', 'Ceiling sensitivity (headroom)'], + [ + FRESHNESS_HALF, + 'Ceiling freshness (checkout vs. base branch)', + // Spelled out rather than spread from HALF_VERDICT: that constant is + // declared below this one, and a spread here would read it in its temporal + // dead zone. `fail` is absent because this half cannot produce one — it + // answers pass / error / not-applicable, and an unexpected status falls + // through to `halfVerdict`'s raw spelling rather than being dressed up. + { pass: '✅ pass', error: '⚠️ superseded ceiling' }, + ], ]; /** @@ -117,7 +139,7 @@ const HALF_VERDICT = { error: '⚠️ broken gauge', }; -const halfVerdict = (status) => HALF_VERDICT[status] ?? `\`${status}\``; +const halfVerdict = (status, verdicts = HALF_VERDICT) => verdicts[status] ?? `\`${status}\``; /** * Renders the per-half breakdown, and renders NOTHING in the two cases where it @@ -130,10 +152,13 @@ const halfVerdict = (status) => HALF_VERDICT[status] ?? `\`${status}\``; * objectui#3152 failure mode this whole file exists to prevent. */ function closureHalfLines(closure) { - const rows = CLOSURE_HALVES.map(([key, label]) => [label, closure[key] ?? '']).filter( - ([, status]) => status !== '', - ); - if (rows.length === 0 || rows.every(([, status]) => status === 'pass')) { + const rows = CLOSURE_HALVES.map(([key, label, verdicts]) => [ + key, + label, + closure[key] ?? '', + verdicts, + ]).filter(([, , status]) => status !== ''); + if (rows.length === 0 || rows.every(([, , status]) => status === 'pass')) { return []; } const lines = [ @@ -141,15 +166,25 @@ function closureHalfLines(closure) { '', '| Eager-closure half | Verdict |', '|--------------------|---------|', - ...rows.map(([label, status]) => `| ${label} | ${halfVerdict(status)} |`), + ...rows.map(([, label, status, verdicts]) => `| ${label} | ${halfVerdict(status, verdicts)} |`), '', ]; - if (rows.some(([, status]) => status === 'error')) { + // The two notes are mutually exclusive per half and BOTH can appear, because + // they say opposite things about the same exit code. Emitting the broken-gauge + // wording for a freshness error would deny a measurement this run actually + // took — the objectui#6230 mistake, one level in. + if (rows.some(([key, , status]) => status === 'error' && key !== FRESHNESS_HALF)) { lines.push( '> ⚠️ A **broken gauge** half is a verdict about the ceiling, not about the bundle: that line has drifted out of range of the regression it exists to catch, or the report behind it cannot be trusted. It does not say anything grew. The `Check console performance budget` step log carries the ceiling and the number it was compared against.', '', ); } + if (rows.some(([key, , status]) => key === FRESHNESS_HALF && status === 'error')) { + lines.push( + '> ⚠️ A **superseded ceiling** is neither a size regression nor a drifted gauge. The bundle was measured correctly and the ceilings on the base branch are correct — but this checkout predates a change to them, so the verdicts above were weighed against numbers that are no longer in force. **Nothing grew.** Update this branch onto the base branch and let `Bundle Analysis` run again; do not widen a ceiling to clear it. The `Check console performance budget` step log names each superseded constant with both values (objectui#6245).', + '', + ); + } return lines; } @@ -284,6 +319,7 @@ export function renderFromEnv(env = process.env, sizeReportPath = 'size-report.m closureChunks: env.BUDGET_CLOSURE_CHUNKS, closureChunkStatus: env.BUDGET_CLOSURE_CHUNK_STATUS, closureHeadroomStatus: env.BUDGET_CLOSURE_HEADROOM_STATUS, + closureFreshnessStatus: env.BUDGET_CLOSURE_FRESHNESS_STATUS, budgetOutcome: env.BUDGET_STEP_OUTCOME, buildOutcome: env.BUILD_PACKAGES_OUTCOME, sizeReport: readSizeReport(sizeReportPath),