Uh oh!
There was an error while loading. Please reload this page.
fix(ci): fail Bundle Analysis when its ceilings were replaced on the base branch - #6315
Merged
Merged
Conversation
…base branch `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe
This was referenced Aug 25, 2026
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe
Contributor
✅ Console Performance Budget
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. 📦 Bundle Size Report
Size Limits
|
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe
Contributor
✅ Console Performance Budget
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. 📦 Bundle Size Report
Size Limits
|
… of inheriting it `Test (shard 3/4)` went red on b1e336a 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CZgmFFzqA9cX8tBMhvpFe
Contributor
✅ Console Performance Budget
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. 📦 Bundle Size Report
Size Limits
|
os-warren
marked this pull request as ready for review
August 25, 2026 13:31
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6245
Four commits. The first adds the freshness half. The second and third make this gate trigger on the files it is built from. The fourth fixes a test-hermeticity defect the first one exposed — see "A false green, and what it cost" below, which is the part most worth reading if you maintain this suite.
The premise, verified before anything was built
The card's framing holds on
origin/main@090927f4f, and I confirmed it from the live run rather than from the card:scripts/check-eager-closure-budget.mjs:179/:188are stillexport const MAX_EAGER_CLOSURE_GZIP_BYTES = 3_345_000;andexport const BASELINE = Object.freeze({...}). The file's last change is0409b766d(test(scripts): derive the eager-closure headroom check from the report, and re-baseline the aggregate ceiling #6229) — the very commit the card names; nothing has touched it since.event: pull_request,conclusion: success,run_started_at: 2026-08-25T03:13:27Z) publishedBUDGET_CLOSURE_BUDGET_KB: 3990.2into its comment step. 3990.2 KiB is 4,085,964 B — the pre-test(scripts): derive the eager-closure headroom check from the report, and re-baseline the aggregate ceiling #6229 ceiling of 4,086,000.0409b766dhad replaced it with 3,345,000 at 03:06:37Z, six minutes and fifty seconds earlier.So a green, merge-gating
Bundle Analysisverdict was computed against a ceiling that had already been retired. It did not bite — the payload was 3222.6 KB, under both numbers — but the aggregate headroom is now 44.0 KB against an 89.0 KB regression class (0.49x), so the next stale verdict has half a regression of room to hide in.The mechanism is in this workflow, not in repository settings:
actions/checkouton apull_requestevent checks out the merge ref, and GitHub does not re-run a PR's checks when the base branch moves.The mechanism, and why this one
evaluateCeilingFreshnessis a fourth half beside the aggregate, per-chunk and sensitivity ones, built on the machinery this file already has rather than beside it.Three readings, not two. The naive check — "do my ceilings match the base branch's?" — fails the one PR that must land: a re-baseline PR differs from the base branch on purpose. So the question is not whether they differ but whether the base branch moved them out from under this checkout:
headSourceprBaseSourcebaseSourceA 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
headSourceonly, the first two agree, and nothing fires.VERDICT_CEILING_CONSTANTSis deliberately just the three constants a verdict is computed from —BASELINEandPER_CHUNK_BASELINEare excluded, so a baseline-only or comment-only edit onmaincannot cry wolf.Exit 2, never exit 1 — and the output says so. Exit 2 already means "a verdict about the GAUGE" here, and the workflow already routes it away from the size verdict. A superseded ceiling is exactly that — nothing grew, no ceiling drifted, the measurement is sound and so are the ceilings on
main; the only thing wrong is that they were never weighed against each other. The step log prints:and the PR comment gets its own verdict —
⚠️ superseded ceiling, not⚠️ broken gauge— with its own note. Reusing the drifted-ceiling wording would have been false in both directions: it denies a measurement that happened, and it points the reader at a report that reads fine. A test pins that the two exit-2 notes stay separate when both halves object.Commits 2 and 3: this gate now triggers on its own runtime closure
The first commit changed this workflow's YAML and none of it could run on the PR that made it. The
paths:filters listedpackages/**,apps/console/**andpnpm-lock.yaml; this PR touches onlyscripts/**and.github/workflows/**. Measured before commit 2: 25 check runs, andBundle Analysiswas not among them.Adding the workflow's own path fixed that and was still not enough: a PR touching only
scripts/check-eager-closure-budget.mjs— the file that computes the verdict — still would not have triggered the gate.The convention on
origin/mainis the gate's runtime closure, not merely its own YAML. Measured across the 7 path-filtered workflows:paths:filterhalf-state-patrol.ymlscripts/invoked-as.mjs, that script's dependencynode-esm-load-gate.ymlpublished-dist-gate.ymlspec-range-floors.ymlhook-selftests.ymlscripts/gatechangeset-guard.ymlperformance-budget.ymlBoth filters now carry what this job actually executes:
invoked-as.mjsis there for a specific reason rather than symmetry:isEntrypointdecides whethermain()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.tsfiles. The closure is what the job executes; this job runs twonode scripts/*.mjscommands and never vitest, so a test-only edit cannot move its verdict, and those tests already run in the root vitestunitproject. A second contract test asserts that definition by deriving the expected set from thenode scripts/*.mjscommands the workflow actually contains, so the list and its rationale cannot drift apart.A false green, and what it cost
⭐ The most transferable thing in this PR. After commit 3,
Test (shard 3/4)went red with four failures — all pre-existing assertions incheck-eager-closure-budget.test.ts, all receiving exit 2 where 0 and 1 were the only outcomes they were written for.The cause was the suite, not the checker. Adding
envtomain(argv, env = process.env)is right for production, but therun()helper omitted the argument and so inherited the process environment.GITHUB_EVENT_NAME=pull_requestis ambient inside GitHub Actions, so in CI — and only in CI — that switched the ceiling-freshness half ON for every case routed throughrun(). With noEAGER_CLOSURE_*_SOURCEinjected, the half correctly returnederrorandmaincorrectly returned 2. Every piece of production code behaved exactly as designed; the test helper was measuring the machine it ran on.So the
vitest run scripts/I reported as green at 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 that helper. The shape of the reverse-verification stands; "green on my machine" never did, for this file. Anyone extending this suite should assume the same trap untilrun()'s hermeticity is verified, which is why it is now a test rather than a comment.Commit 4 makes
run()inject{ GITHUB_OUTPUT: outputPath, ...env }and stop touchingprocess.envaltogether.GITHUB_EVENT_NAMEis deliberately absent from that object — that is what makes these cases exercise the non-pull_request path deterministically rather than by luck; a case opts in through the newenvparameter. The production default is untouched, and none of the four assertions were changed to expect 2, which would have pinned the accident instead of fixing it.What this does NOT close, stated plainly
It closes the window the 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-mainrun detects after the fact, and closing it needs option 1 in the card body (require branches up to date before merging), a repository setting and maintainer-floor. Not implemented, not proposed, no repository setting touched, no required context added, no ceiling widened. Tracked as #6319.Files
scripts/check-eager-closure-budget.mjs—VERDICT_CEILING_CONSTANTS,extractCeilingDeclarations,evaluateCeilingFreshness,readSource;main(argv, env)takes an injected env and threads it towriteGithubOutput..github/workflows/performance-budget.yml—fetch-depth: 2, theResolve the base-branch ceiling constantsstep,BUDGET_CLOSURE_FRESHNESS_STATUSinto the comment step, an exit-2budget_messagethat no longer claims "no trustworthy measurement", and bothpaths:filters carrying the runtime closure.scripts/render-budget-comment.mjs— the fourth half row, its own verdict wording and note.scripts/__tests__/files — 28 new assertions, andrun()made hermetic.continue-on-error: trueon the resolve step is not a fail-open: when the step cannot produce both files the checker finds the variables unset and reports a freshness ERROR (exit 2), which fails the job. Failing the step itself would kill the run before the bundle is measured (#3152). The paths travel through$GITHUB_ENVrather than a stepenv:block becauserender-budget-comment.test.tswalks everyNAME: ${{ steps.* }}line and requires the comment renderer to read it.Verification
CI on
b6cfa76f7— every job greenTest (shard 3/4)— the one that was redRun tests (shard 3/4): success)Testshards 1/4, 2/4, 4/4Type Check(incl.Type-check scripts/)Build & E2E,Lint,Build DocsBundle Analysis(run 32852342807)The freshness half, measured on CI in its discriminating case
Bundle Analysisrun 32850298987 onb1e336a50caught the case that matters. Quoted from its log:The two SHAs differ —
mainmoved under this PR while it was open — so the half had to decide whether any ceiling moved with it. Confirmed independently:mainadvanced 7 commits across that span,git diffover the three closure files is empty, and both ceiling constants are byte-identical at each end.passis therefore the correct answer on a genuinely moved base, not an accidental one — the half does not cry wolf on ordinary base movement. (The earlier run on11d60f84ahad both SHAs equal and exercised only the degenerate path.)One honest limit: the checker's own stdout prose is not quotable from my seat — the job-log API truncates to ~4.4 KB and the full-log archive host is blocked by this container's proxy (403 on CONNECT). What I quote is the published output value,
freshness.statusfrom the same verdict object; I have not read the prose line and am not presenting it as read.Red before, green after
The freshness half, reverse-verified from the committed state with the three production files reverted to
origin/main, tests kept. Mutation proven on disk by grep counts and blob hash; restore proven byte-identical toHEAD. A behavioural probe using only exports that exist onorigin/main:closure_freshness_statuserrorpasspassThe two test files under that ablation: 25 failed — behavioural, not import errors.
Each contract test ablated separately from its own committed state:
- '.github/workflows/performance-budget.yml'entries deleted (2 → 0)- 'scripts/check-eager-closure-budget.mjs'deleted from both filters (2 → 0)__tests__path injected intoTRIGGER_CLOSURErun()reverted to inheritingprocess.env, no ambient variable setThat last row is the point of the guard: it reds locally, on the change that causes it, rather than on someone else's PR.
Hermeticity, checked in both directions
The whole
scripts/suite atb6cfa76f7, exit codes captured by redirect before any pipe:GITHUB_EVENT_NAME=pull_request(reproduces CI)Tests 2262 passed (2262)Tests 2262 passed (2262)$GITHUB_OUTPUTTests 2262 passed (2262); decoy file never writtenSetting the variable is what turns "it passes here" into a measurement — on the parent commit, the first row is exactly the run that reproduces CI's four failures.
Gates
All at
b6cfa76f7, clean tree, exit codes by redirect before any pipe:pnpm type-check:scriptspnpm lint:root(full root population)✖ 28 problems (0 errors, 28 warnings)— 0 in the changed filescheck-control-bytes✅ OK (scanned 5195 tracked text file(s); skipped 85 binary)check-entry-guard✓ 47 scripts/ file(s) … 42 export bindings, 42 of them inert on importcheck-shell-escape-residue✅ OK (4/4 root(s) resolved …)check-changeset-presence✅ No source of a released package changed in this range, so no changeset is owed.check-pre-install-import-graph✅ OK — 16 pre-install step(s) in 15 job(s) …No changeset, on the gate's own verdict.
objectstack-ai/objectuihas noskip-changesetlabel mechanism, so nothing was labelled.Generated by Claude Code