From af60a25b871489fc4d3a9251defa29ffdbf6bcf7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 03:36:29 +0000 Subject: [PATCH] fix(ci): name which eager-closure half objected in the budget PR comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check-eager-closure-budget.mjs` publishes a verdict for each of its three halves — `closure_status` (aggregate ceiling), `closure_chunk_status` (per-chunk ceilings) and `closure_headroom_status` (ceiling sensitivity) — but `performance-budget.yml` passed only the first into the comment step. The other two were written to `$GITHUB_OUTPUT` and never read. The step's exit code folds all three into one `budget_status`, so the comment could say a budget objected but not which half did. In the shape the gate family exists for — one chunk over its own ceiling while the total sits inside the aggregate one — both metrics the comment prints are green and the verdict is still FAIL, with nothing in the body explaining it. - Wire both missing verdicts into the comment step and render them. - Render nothing when every half passed: the healthy comment is byte-for-byte what it was. An observability change that rewrites the green output is a regression. - Exit 2 lands in the not-measured branch, so render the halves there too, and stop asserting "nothing was measured" when the closure was measured and only a ceiling drifted. - Pin the obligation: the suite fails if the checker publishes a `closure_*` verdict the workflow does not pass through. The exit-code mapping is untouched — every `budget_status=` and `CLOSURE_CODE` line is byte-identical. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019b5UBNMtTzKbVtZZGvFuxe --- .changeset/6230-budget-comment-half-status.md | 29 ++++ .github/workflows/performance-budget.yml | 17 +- .../__tests__/render-budget-comment.test.ts | 163 ++++++++++++++++++ scripts/render-budget-comment.mjs | 119 ++++++++++++- 4 files changed, 318 insertions(+), 10 deletions(-) create mode 100644 .changeset/6230-budget-comment-half-status.md diff --git a/.changeset/6230-budget-comment-half-status.md b/.changeset/6230-budget-comment-half-status.md new file mode 100644 index 0000000000..3b79f6fff9 --- /dev/null +++ b/.changeset/6230-budget-comment-half-status.md @@ -0,0 +1,29 @@ +--- +--- + +CI-only: the Bundle Analysis PR comment now names *which* eager-closure half +objected, instead of reporting only that something did. + +`scripts/check-eager-closure-budget.mjs` evaluates three halves and publishes a +verdict for each to `$GITHUB_OUTPUT` — `closure_status` (the aggregate ceiling), +`closure_chunk_status` (the per-chunk ceilings) and `closure_headroom_status` +(ceiling sensitivity). `.github/workflows/performance-budget.yml` passed only +the first into the comment step, so the two others were published and never +read. The step's exit code folds all three into one `budget_status`, which meant +the comment could say a budget objected but not which half — a reader had to +open the job log to learn whether the total grew, one chunk grew, or a ceiling +had stopped measuring anything. + +- Both missing verdicts are now passed into the comment step and rendered. +- The healthy comment is unchanged: the breakdown appears only when a half is + not `pass`, verified byte-for-byte against the previous renderer. +- A drifted ceiling (exit 2) reads as a broken **gauge** rather than a size + failure, and no longer claims "nothing was measured" while showing two + ceilings that passed. +- `render-budget-comment.test.ts` now fails if the checker publishes a + `closure_*` verdict the workflow does not wire through, so a fourth half + cannot repeat this. + +The exit-code mapping is untouched: exit 2 still maps to `budget_status=error`, +any other non-zero to `fail`, and `error` still outranks `fail` across all three +halves. diff --git a/.github/workflows/performance-budget.yml b/.github/workflows/performance-budget.yml index 330b52b235..0a5953bd1c 100644 --- a/.github/workflows/performance-budget.yml +++ b/.github/workflows/performance-budget.yml @@ -152,7 +152,16 @@ jobs: echo "" echo "📦 Eager closure (what a page load actually pays for):" # Writes closure_status / closure_gzip_kb / closure_budget_kb / - # closure_chunks to $GITHUB_OUTPUT itself. Exit codes are distinct on + # closure_chunks / closure_chunk_status / closure_headroom_status to + # $GITHUB_OUTPUT itself. EVERY key it publishes is passed into the + # comment step below and rendered there — a verdict published to + # $GITHUB_OUTPUT that no consumer reads is a verdict the PR comment + # 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. + # + # Exit codes are distinct on # purpose: 1 = over budget (a verdict about the bundle), 2 = no # trustworthy measurement (a verdict about the gauge). Reporting one as # the other is how a broken gauge gets read as a size regression, and a @@ -243,6 +252,12 @@ jobs: BUDGET_CLOSURE_GZIP_KB: ${{ steps.budget.outputs.closure_gzip_kb }} BUDGET_CLOSURE_BUDGET_KB: ${{ steps.budget.outputs.closure_budget_kb }} BUDGET_CLOSURE_CHUNKS: ${{ steps.budget.outputs.closure_chunks }} + # The per-chunk (objectui#5490) and sensitivity (objectui#5924) halves. + # The step's exit code folds all three halves into one `budget_status`, + # so without these the comment can say a budget objected but not which + # 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 }} 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__/render-budget-comment.test.ts b/scripts/__tests__/render-budget-comment.test.ts index 1e713c5757..76a79c7f7a 100644 --- a/scripts/__tests__/render-budget-comment.test.ts +++ b/scripts/__tests__/render-budget-comment.test.ts @@ -11,6 +11,7 @@ import { renderBudgetComment, renderFromEnv } from '../render-budget-comment.mjs const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); const workflowPath = path.join(repoRoot, '.github/workflows/performance-budget.yml'); const rendererPath = path.join(repoRoot, 'scripts/render-budget-comment.mjs'); +const checkerPath = path.join(repoRoot, 'scripts/check-eager-closure-budget.mjs'); const NO_SUCH_REPORT = path.join(repoRoot, 'scripts/__tests__/__no-size-report__.md'); /** @@ -190,6 +191,142 @@ describe('renderBudgetComment', () => { }); }); +/** + * objectui#6230: the closure checker evaluates THREE halves — the aggregate + * ceiling, the per-chunk ceilings (objectui#5490) and ceiling sensitivity + * (objectui#5924) — and publishes a verdict for each. The step's exit code + * folds all three into one `budget_status`, so a comment rendering only that + * says "something objected" and nothing more: a reader had to open the job log + * to learn whether the total grew, one chunk grew, or a ceiling had stopped + * measuring anything. + * + * The per-chunk case is the one the whole gate family exists for. objectui#5266 + * was a 89 KiB regression that landed OUTSIDE the entry chunk and inside the + * aggregate ceiling's headroom — exactly the shape where both numbers the + * comment prints are green and the verdict is still FAIL. + */ +describe('per-half closure verdicts', () => { + const halves = { + gzipKb: '28.1', + budgetKb: '350', + entryFile: 'index-BRKCVm_4.js', + closureGzipKb: '3222.6', + closureBudgetKb: '3266.6', + closureChunks: '58', + closureStatus: 'pass', + closureChunkStatus: 'pass', + closureHeadroomStatus: 'pass', + }; + + /** + * The load-bearing leg. An observability change that rewrites the HEALTHY + * output is a regression: every green PR comment in the repo would change + * shape to report nothing new. + */ + it('adds nothing at all to the comment when every half passed', () => { + const { kind, body } = renderBudgetComment({ status: 'pass', ...halves }); + + expect(kind).toBe('pass'); + expect(body).not.toContain('Which half objected'); + expect(body).not.toContain('Eager-closure half'); + expect(body).toContain('| Status | **PASS** | — |'); + expect(body).not.toContain('FAIL'); + }); + + it('names the AGGREGATE half when the total is over its ceiling', () => { + const { body } = renderBudgetComment({ + status: 'fail', + ...halves, + closureGzipKb: '3400.0', + closureStatus: 'fail', + }); + + expect(body).toContain('**Which half objected:**'); + expect(body).toContain('| Aggregate closure ceiling | ❌ over its ceiling |'); + expect(body).toContain('| Per-chunk ceilings | ✅ pass |'); + expect(body).toContain('| Ceiling sensitivity (headroom) | ✅ pass |'); + }); + + /** + * The objectui#5266 shape. Note the numbers here are IDENTICAL to the + * all-pass case above: before this wiring the two comments differed only in + * `PASS` vs `FAIL`, with both printed metrics comfortably inside budget and + * nothing anywhere in the body saying why. + */ + it('names the PER-CHUNK half when one chunk is over while the total is not', () => { + const { kind, body } = renderBudgetComment({ + status: 'fail', + ...halves, + closureChunkStatus: 'fail', + }); + + expect(kind).toBe('fail'); + // Both printed metrics are green; only the half table explains the FAIL. + expect(body).toContain('| **Eager closure** (gzip, 58 chunks) | **3222.6 KB** | 3266.6 KB |'); + expect(body).toContain('| Status | **FAIL** | — |'); + expect(body).toContain('| Per-chunk ceilings | ❌ over its ceiling |'); + expect(body).toContain('| Aggregate closure ceiling | ✅ pass |'); + }); + + /** + * A drifted ceiling is exit 2, which `performance-budget.yml` maps to + * `budget_status=error` — so it lands in the NOT-MEASURED branch, not the + * verdict branch. It must read as a verdict about the gauge, and must never + * acquire a ❌: nothing grew. + */ + it('names a drifted ceiling as a broken GAUGE, never as a size failure', () => { + const { kind, body } = renderBudgetComment({ + status: 'error', + ...halves, + closureHeadroomStatus: 'error', + message: 'the eager-closure gauge produced no trustworthy measurement', + budgetOutcome: 'failure', + buildOutcome: 'success', + }); + + expect(kind).toBe('not-measured'); + expect(body).toContain('| Ceiling sensitivity (headroom) | ⚠️ broken gauge |'); + expect(body).toContain('a verdict about the ceiling, not about the bundle'); + expect(body).not.toContain('❌'); + expect(body).not.toContain('FAIL'); + // The closure WAS measured on this path, so the comment must not claim the + // opposite two lines above a table showing two ceilings that passed. + expect(body).not.toContain('Nothing was measured'); + expect(body).toContain('gauge not trustworthy'); + }); + + it('keeps the "not measured" wording when nothing really was measured', () => { + // dist missing / no JS / cancelled: the checker never ran, so it published + // no closure numbers. This branch must be untouched by the above. + const { kind, body } = renderBudgetComment({ + status: 'error', + message: 'Build output not found at apps/console/dist/assets', + budgetOutcome: 'failure', + buildOutcome: 'success', + }); + + expect(kind).toBe('not-measured'); + expect(body).toContain('## ℹ️ Console Performance Budget — not measured'); + expect(body).toContain('Nothing was measured'); + expect(body).not.toContain('Which half objected'); + }); + + 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. + const { body } = renderBudgetComment({ + status: 'fail', + ...halves, + closureStatus: '', + closureChunkStatus: '', + closureHeadroomStatus: '', + }); + + expect(body).not.toContain('Which half objected'); + expect(body).not.toContain('| Per-chunk ceilings |'); + }); +}); + describe('renderFromEnv', () => { it('reads the exact env names the workflow sets', () => { const { kind, body } = renderFromEnv( @@ -283,6 +420,32 @@ describe('performance-budget.yml contract', () => { } }); + /** + * objectui#6230: `closure_chunk_status` (objectui#5490) and + * `closure_headroom_status` (objectui#5924) were both published to + * `$GITHUB_OUTPUT` and never read, each following the precedent of the last. + * The card's explicit ask was that a THIRD round of this not happen — so the + * obligation is pinned mechanically rather than left to review: a half added + * to the checker fails here until it reaches the comment. + */ + it('passes every closure verdict the checker publishes into the comment step', () => { + const checker = fs.readFileSync(checkerPath, 'utf8'); + const call = checker.slice(checker.lastIndexOf('writeGithubOutput({')); + const published = [...call.slice(0, call.indexOf('});')).matchAll(/(closure_\w+):/g)].map( + (m) => m[1], + ); + + // Guard the guard: a regex that matched nothing would pass silently. + expect(published).toContain('closure_status'); + expect(published).toContain('closure_chunk_status'); + expect(published).toContain('closure_headroom_status'); + + for (const key of published) { + expect(workflow, `workflow must pass steps.budget.outputs.${key} to the comment step`) + .toContain(`steps.budget.outputs.${key}`); + } + }); + 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/render-budget-comment.mjs b/scripts/render-budget-comment.mjs index a80554435b..55c6ad720c 100644 --- a/scripts/render-budget-comment.mjs +++ b/scripts/render-budget-comment.mjs @@ -48,6 +48,8 @@ const text = (value) => (typeof value === 'string' ? value.trim() : ''); * @param {string} [input.closureGzipKb] measured gzip size of the eager closure, in KB * @param {string} [input.closureBudgetKb] the closure ceiling it was compared against * @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.message] human-readable reason when `status` is `error` * @param {string} [input.budgetOutcome] `steps.budget.outcome` * @param {string} [input.buildOutcome] `steps.build_packages.outcome` @@ -66,6 +68,8 @@ export function renderBudgetComment(input = {}) { gzipKb: text(input.closureGzipKb), budgetKb: text(input.closureBudgetKb), chunks: text(input.closureChunks), + chunkStatus: text(input.closureChunkStatus), + headroomStatus: text(input.closureHeadroomStatus), }; // A verdict needs an affirmative status AND the numbers that status was @@ -82,11 +86,73 @@ export function renderBudgetComment(input = {}) { buildOutcome: text(input.buildOutcome), sizeReport, runUrl: text(input.runUrl), + closure, }); return { kind: measured ? status : 'not-measured', body }; } +/** + * 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 + * 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. + */ +const CLOSURE_HALVES = [ + ['status', 'Aggregate closure ceiling'], + ['chunkStatus', 'Per-chunk ceilings'], + ['headroomStatus', 'Ceiling sensitivity (headroom)'], +]; + +/** + * `error` is deliberately NOT worded as a size verdict. It is what exit 2 means + * in the checker: the report cannot be trusted, or a ceiling has drifted out of + * range of the regression it must catch. Nothing grew. + */ +const HALF_VERDICT = { + pass: '✅ pass', + fail: '❌ over its ceiling', + error: '⚠️ broken gauge', +}; + +const halfVerdict = (status) => HALF_VERDICT[status] ?? `\`${status}\``; + +/** + * Renders the per-half breakdown, and renders NOTHING in the two cases where it + * would be noise: + * + * - every half passed — the healthy comment stays byte-for-byte what it was. + * An observability change that rewrites the green output is a regression; + * - no half status was handed over — a caller that does not pass them gets + * silence, never a table of blanks inferred into verdicts, which is the + * 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')) { + return []; + } + const lines = [ + '**Which half objected:**', + '', + '| Eager-closure half | Verdict |', + '|--------------------|---------|', + ...rows.map(([label, status]) => `| ${label} | ${halfVerdict(status)} |`), + '', + ]; + if (rows.some(([, status]) => status === 'error')) { + 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.', + '', + ); + } + return lines; +} + function verdictBody({ status, gzipKb, budgetKb, entryFile, sizeReport, closure }) { const pass = status === 'pass'; const closureMeasured = closure.gzipKb !== '' && closure.budgetKb !== ''; @@ -108,6 +174,7 @@ function verdictBody({ status, gzipKb, budgetKb, entryFile, sizeReport, closure // gate (objectui#5324). 'The **eager closure** is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it.', '', + ...closureHalfLines(closure), ]; if (!closureMeasured) { // Never let an absent closure figure render as a quiet table with one row. @@ -120,20 +187,52 @@ function verdictBody({ status, gzipKb, budgetKb, entryFile, sizeReport, closure return withSizeReport(lines.join('\n'), sizeReport); } -function notMeasuredBody({ message, budgetOutcome, buildOutcome, sizeReport, runUrl }) { - const lines = [ - '## ℹ️ Console Performance Budget — not measured', - '', - 'This run did not produce a console bundle to measure, so there is **no pass/fail verdict** for the performance budget.', - '', - '**This is not a budget violation.** Nothing was measured — the numbers a real violation would carry are simply absent.', - '', +function notMeasuredBody({ + message, + budgetOutcome, + buildOutcome, + sizeReport, + runUrl, + closure = {}, +}) { + // objectui#6230: `error` does not always mean "nothing was measured". A + // ceiling that has drifted out of range of the regression it must catch is + // exit 2 with a perfectly good measurement behind it, and then the "nothing + // was measured" wording below is FALSE — the comment would deny a measurement + // in the same breath as the half table showing two ceilings that passed on + // one. Discriminate on the same emptiness the verdict branch keys on: the + // checker publishes `closure_gzip_kb` EMPTY when it has no report, never as a + // stale number, and that is pinned by its own tests. + const closureMeasured = (closure.gzipKb ?? '') !== '' && (closure.budgetKb ?? '') !== ''; + const lines = closureMeasured + ? [ + '## ⚠️ Console Performance Budget — gauge not trustworthy', + '', + 'The eager closure was measured, but one of the ceilings it is measured against no longer means what it names, so this run carries **no pass/fail verdict** for the performance budget.', + '', + '**This is not a budget violation.** Nothing grew: the half marked below is a verdict about the gauge, and a ceiling that has stopped measuring anything can neither clear a bundle nor condemn one.', + '', + ] + : [ + '## ℹ️ Console Performance Budget — not measured', + '', + 'This run did not produce a console bundle to measure, so there is **no pass/fail verdict** for the performance budget.', + '', + '**This is not a budget violation.** Nothing was measured — the numbers a real violation would carry are simply absent.', + '', + ]; + lines.push( '| Step | Outcome |', '|------|---------|', `| Build packages | \`${outcome(buildOutcome)}\` |`, `| Check console performance budget | \`${outcome(budgetOutcome)}\` |`, '', - ]; + // Exit 2 — a drifted ceiling, or a report that cannot be trusted — maps to + // `budget_status=error`, and error lands HERE rather than in the verdict + // body. So this branch needs the halves as much as that one does: they are + // the only thing in the comment that says WHICH gauge is broken. + ...closureHalfLines(closure), + ); if (message) { lines.push(`Reason: ${message}`, ''); @@ -183,6 +282,8 @@ export function renderFromEnv(env = process.env, sizeReportPath = 'size-report.m closureGzipKb: env.BUDGET_CLOSURE_GZIP_KB, closureBudgetKb: env.BUDGET_CLOSURE_BUDGET_KB, closureChunks: env.BUDGET_CLOSURE_CHUNKS, + closureChunkStatus: env.BUDGET_CLOSURE_CHUNK_STATUS, + closureHeadroomStatus: env.BUDGET_CLOSURE_HEADROOM_STATUS, budgetOutcome: env.BUDGET_STEP_OUTCOME, buildOutcome: env.BUILD_PACKAGES_OUTCOME, sizeReport: readSizeReport(sizeReportPath),