From cbe6c799eb04f4dd3317f7f6f96f0c2854109d32 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 12:24:05 +0000 Subject: [PATCH 1/4] fix(ci): fail Bundle Analysis when its ceilings were replaced on the base branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Bundle Analysis` is a required context, and a `pull_request` run checks out the MERGE REF — the PR head merged 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 `main` has since replaced, and the merge is then gated on a verdict about a ceiling that no longer exists. Measured, not inferred. Run 32804357171 (`pull_request`, `conclusion: success`) started at 03:13:27Z, 6m50s after `0409b766d` lowered 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. It did not bite: the payload was 3222.6 KB, under both numbers. The aggregate headroom is now 44.0 KB against an 89.0 KB regression class, so the next stale verdict has half a regression of room to hide in. `evaluateCeilingFreshness` is a fourth half alongside the aggregate, per-chunk and sensitivity ones, and it compares VERDICT_CEILING_CONSTANTS across THREE readings of this file: this checkout, the base commit the checkout was computed from, and the base branch tip. Two readings 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. It is exit 2, a verdict about the GAUGE, never exit 1: nothing grew, no ceiling has drifted, and the message says so in as many words. A freshness failure that read like "the bundle grew" would send its reader hunting a regression that is not there — and the repair they would reach for is widening a ceiling, the one thing this must never teach. The PR comment gets its own `superseded ceiling` verdict and its own note rather than borrowing the broken-gauge wording, which would be false here. What this does not close: a run that FINISHES before the constants move and 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. Fixes #6245 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe --- .github/workflows/performance-budget.yml | 81 +++- .../check-eager-closure-budget.test.ts | 315 +++++++++++++ .../__tests__/render-budget-comment.test.ts | 105 +++++ scripts/check-eager-closure-budget.mjs | 413 +++++++++++++++++- scripts/render-budget-comment.mjs | 64 ++- 5 files changed, 948 insertions(+), 30 deletions(-) diff --git a/.github/workflows/performance-budget.yml b/.github/workflows/performance-budget.yml index 0a5953bd1c..1fee4e8c51 100644 --- a/.github/workflows/performance-budget.yml +++ b/.github/workflows/performance-budget.yml @@ -31,6 +31,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 +68,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 +231,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 +247,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 +332,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..403f8689b4 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 @@ -761,6 +765,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..bf94b322d1 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,49 @@ 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}`); + } + }); + + 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), From 11d60f84a2013b8d8176d2db18a621eb6e2b06e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 12:37:35 +0000 Subject: [PATCH 2/4] fix(ci): trigger Bundle Analysis on its own workflow file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit changed this gate's YAML — `fetch-depth: 2`, the resolve step, the `$GITHUB_ENV` threading, `BUDGET_CLOSURE_FRESHNESS_STATUS` reaching the comment step — and none of it could ever run on the PR that made it. The `paths:` filters list `packages/**`, `apps/console/**` and `pnpm-lock.yaml`; a PR editing only `scripts/**` and `.github/workflows/**` matches none of them, so `Bundle Analysis` never appears. Measured on PR #6315 before this commit: 25 check runs, and `Bundle Analysis` was not among them. That is worse here than it looks, precisely because the freshness half is fail-closed by design. A wiring bug in the resolve step yields missing sources, freshness `error`, exit 2 — correctly loud, but the red would land on the NEXT `packages/**` PR, which belongs to another seat, and it would read to them as a bundle problem on their own diff. Self-inclusion is this repo's convention, not a new policy. Measured on `origin/main` @ `090927f4f`: 29 workflows, 7 carry a `paths:` filter, and 5 of those 7 list their own file (`half-state-patrol`, `hook-selftests`, `node-esm-load-gate`, `published-dist-gate`, `spec-range-floors`). This gate was one of the two exceptions. The cost is real and accepted: every future edit of this file now pays a console build, the same tax the five already pay. The contract test pins it for both the `push` and `pull_request` filters, so the exception cannot quietly return. Part of #6245 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe --- .github/workflows/performance-budget.yml | 24 ++++++++++++++++ .../__tests__/render-budget-comment.test.ts | 28 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/.github/workflows/performance-budget.yml b/.github/workflows/performance-budget.yml index 1fee4e8c51..d55b98353b 100644 --- a/.github/workflows/performance-budget.yml +++ b/.github/workflows/performance-budget.yml @@ -7,12 +7,36 @@ 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' 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' concurrency: group: bundle-analysis-${{ github.event.pull_request.number || github.ref }} diff --git a/scripts/__tests__/render-budget-comment.test.ts b/scripts/__tests__/render-budget-comment.test.ts index bf94b322d1..a2d62fcf4d 100644 --- a/scripts/__tests__/render-budget-comment.test.ts +++ b/scripts/__tests__/render-budget-comment.test.ts @@ -541,6 +541,34 @@ describe('performance-budget.yml contract', () => { } }); + /** + * objectui#6245, second pass. The first version of this change shipped a + * workflow half — `fetch-depth: 2`, the resolve step, the `$GITHUB_ENV` + * threading — that `Bundle Analysis` never ran on, because the PR editing it + * touched no `packages/**` path and so the gate never appeared on its own + * PR. A wiring bug there is fail-CLOSED by design, which is right, but it + * means the red would have surfaced on the NEXT `packages/**` PR and read to + * that seat as a bundle problem on their own diff. + * + * Self-inclusion is this repo's convention rather than a new policy: + * measured on `origin/main`, 5 of the 7 workflows carrying a `paths:` filter + * list their own file, and this one was an exception. + */ + it('triggers on its own file, so a change to this gate is exercised by its own PR', () => { + const selfPath = '.github/workflows/performance-budget.yml'; + // `#` 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. + expect(filters).toHaveLength(2); + for (const filter of filters) { + expect(filter, 'every paths: filter must list this workflow itself').toContain(selfPath); + } + }); + 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). From b1e336a5055a65fcb7786903d9a43c3c6d4813ba Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 12:53:32 +0000 Subject: [PATCH 3/4] fix(ci): trigger Bundle Analysis on its whole runtime closure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Listing this workflow's own YAML in its `paths:` filters was necessary and still not sufficient. A PR touching only `scripts/check-eager-closure-budget.mjs` — the file that computes the verdict, and the file this card is about — did not trigger `Bundle Analysis`. The previous commit's PR triggered it only because it also edited the YAML; the next person to edit the checker alone would have hit exactly the gap that commit closed. The convention on `origin/main` is the gate's RUNTIME CLOSURE, not merely its own workflow file. Measured across the 7 path-filtered workflows: `half-state-patrol` lists the script it runs AND `scripts/invoked-as.mjs`, that script's dependency; `node-esm-load-gate`, `published-dist-gate` and `spec-range-floors` each list their gate script by exact path. So both filters now carry what this job actually executes: - scripts/check-eager-closure-budget.mjs - scripts/render-budget-comment.mjs - scripts/invoked-as.mjs `invoked-as.mjs` is the third entry, and it is here for a specific reason rather than for symmetry with `half-state-patrol`: `isEntrypoint` decides whether `main()` runs at all, so a regression in it makes this checker exit 0 having measured nothing — the silent-green failure the rest of this gate exists to prevent. NOT listed: the `scripts/__tests__/*.test.ts` files. The closure is defined by what the job EXECUTES, and this job runs two `node scripts/*.mjs` commands and never vitest, so a test-only edit cannot move its verdict; 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. Two contract tests hold it: one pins every closure entry in BOTH filters, the other derives the expected set from the `node scripts/*.mjs` commands the workflow actually contains, so the list and its definition cannot drift apart. Part of #6245 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe --- .github/workflows/performance-budget.yml | 44 +++++++++++++ .../__tests__/render-budget-comment.test.ts | 64 +++++++++++++++---- 2 files changed, 94 insertions(+), 14 deletions(-) diff --git a/.github/workflows/performance-budget.yml b/.github/workflows/performance-budget.yml index d55b98353b..6b4f608a91 100644 --- a/.github/workflows/performance-budget.yml +++ b/.github/workflows/performance-budget.yml @@ -19,6 +19,28 @@ on: # 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: @@ -37,6 +59,28 @@ on: # 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 }} diff --git a/scripts/__tests__/render-budget-comment.test.ts b/scripts/__tests__/render-budget-comment.test.ts index a2d62fcf4d..8588fb19db 100644 --- a/scripts/__tests__/render-budget-comment.test.ts +++ b/scripts/__tests__/render-budget-comment.test.ts @@ -542,31 +542,67 @@ describe('performance-budget.yml contract', () => { }); /** - * objectui#6245, second pass. The first version of this change shipped a - * workflow half — `fetch-depth: 2`, the resolve step, the `$GITHUB_ENV` - * threading — that `Bundle Analysis` never ran on, because the PR editing it - * touched no `packages/**` path and so the gate never appeared on its own - * PR. A wiring bug there is fail-CLOSED by design, which is right, but it - * means the red would have surfaced on the NEXT `packages/**` PR and read to - * that seat as a bundle problem on their own diff. + * 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. * - * Self-inclusion is this repo's convention rather than a new policy: - * measured on `origin/main`, 5 of the 7 workflows carrying a `paths:` filter - * list their own file, and this one was an exception. + * 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. */ - it('triggers on its own file, so a change to this gate is exercised by its own PR', () => { - const selfPath = '.github/workflows/performance-budget.yml'; + 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. + // 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) { - expect(filter, 'every paths: filter must list this workflow itself').toContain(selfPath); + 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', () => { From b6cfa76f74f62b2c6b9e6b5356f7a388b32cf6f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 13:14:06 +0000 Subject: [PATCH 4/4] test(scripts): inject main()'s env in the eager-closure suite instead of inheriting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Test (shard 3/4)` went red on b1e336a50 with four failures, all pre-existing assertions in this file, all receiving exit 2 where 0 and 1 were the only outcomes they were written for. The cause is this suite, not the checker. Adding `env` to `main(argv, env = process.env)` was right for production, but the `run()` helper here omitted the argument and so inherited the process environment. `GITHUB_EVENT_NAME=pull_request` is ambient inside GitHub Actions, so in CI — and only in CI — that switched the ceiling-freshness half ON for every case routed through `run()`. With no `EAGER_CLOSURE_*_SOURCE` injected, the half correctly reported `error` and `main` correctly returned 2. Both halves of the machinery behaved exactly as designed; the test helper was measuring the machine it ran on. ⭐ Worth stating plainly, because it is the reusable lesson: the local `vitest run scripts/` that reported 2261 passing was a FALSE GREEN. It passed because a variable happened to be absent, not because the code was right, and the same is true of every ablation routed through this helper. "Green on my machine" was never evidence for this file. `run()` now injects `{ GITHUB_OUTPUT: outputPath, ...env }` and no longer mutates `process.env` at all. `GITHUB_EVENT_NAME` is deliberately absent from that object, which is what makes these cases exercise the non-pull_request path deterministically rather than by luck; a case can opt in through the new `env` parameter. The production default is untouched — `env = process.env` is correct — and none of the four assertions were changed to expect 2, which would have pinned the accident instead of fixing it. A new test sets `GITHUB_EVENT_NAME=pull_request` in-process and asserts `run()` is unmoved by it, so a regression to inheritance reds locally on the change that causes it rather than on someone else's unrelated PR. Verified in both directions rather than one: the whole `scripts/` suite passes with the variable ambient (which reproduces the CI failure on the parent commit), with it unset, and under a fuller fake-Actions env whose decoy `$GITHUB_OUTPUT` the suite never writes to. Part of #6245 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe --- .../check-eager-closure-budget.test.ts | 50 ++++++++++++++++--- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/scripts/__tests__/check-eager-closure-budget.test.ts b/scripts/__tests__/check-eager-closure-budget.test.ts index 403f8689b4..9a07b5fc23 100644 --- a/scripts/__tests__/check-eager-closure-budget.test.ts +++ b/scripts/__tests__/check-eager-closure-budget.test.ts @@ -565,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') @@ -583,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.