Conversation
|
Updated 7:54 AM PT - Sep 6th, 2026
❌ @robobun, your commit 1437e4a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35906That installs a local version of the PR into your bun-35906 --bun |
WalkthroughChangesBinary size comparisons now resolve PR baselines from merge-base history, support artifact and metadata fallback, track stale triplets, update reporting, and add integration coverage for stale and failing scenarios. Binary size baseline handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/binary-size.ts`:
- Line 183: Update the numeric presence checks in the build-number comparison
and the base-size return path, using explicit absence checks rather than
truthiness so legitimate zero values remain valid. Specifically adjust the
conditions around n and base while preserving the existing comparison and
undefined-return behavior for actually missing values.
- Around line 149-153: Validate the parsed value in the loop over want before
assigning it to sizes: store the result of parseInt(v, 10), reject or otherwise
handle NaN, and only add valid numeric sizes to sizes[triplet]. Preserve the
existing undefined return when no valid sizes remain.
- Around line 134-136: Update the temporary directory setup around dir in the
binary-size script to resolve binary-size-tmp to an absolute path, preferably
beneath the system temporary directory. Keep the existing rmSync and mkdirSync
lifecycle unchanged while ensuring each operation uses the resolved absolute
directory.
- Around line 113-117: Add per-request timeout signals to the fetch calls in
githubJson and the corresponding Buildkite request path. Configure each request
with a finite timeout so GitHub and Buildkite canary fetches fail promptly
rather than hanging during the commit walk.
In `@test/internal/binary-size-baseline.test.ts`:
- Around line 159-187: Change both independent binary-size tests to use
test.concurrent while preserving their existing skipIf POSIX conditions, test
bodies, and assertions. Update the declarations around the tests describing
stale baselines and merge-base threshold failures; no other behavior changes are
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5180e016-e52d-4533-9c4a-d0bda87ff766
📒 Files selected for processing (2)
scripts/binary-size.tstest/internal/binary-size-baseline.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/binary-size.ts (1)
55-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEncode
baseBranchbefore using it as theshaquery value.A valid ref may contain
#or&; raw interpolation intocommits?sha=${walkFrom}can truncate or alter the query and select an unrelated baseline. Encode the query value at the request sink.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/binary-size.ts` around lines 55 - 56, Update the request construction in the binary-size flow to URL-encode the baseBranch-derived sha query value before interpolating it into the commits request. Apply encoding at the request sink while preserving the existing baseBranch fallback and baseline-selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/binary-size.ts`:
- Around line 154-155: Update the metadata parsing around the bytes assignment
in scripts/binary-size.ts to validate the complete trimmed decimal
representation before converting it, rejecting values with trailing or otherwise
invalid characters. Record sizes[triplet] only when the parsed value passes
Number.isSafeInteger(), while preserving the existing handling for missing
values.
- Line 205: For PR baseline evaluation, update the anchor logic in
scripts/binary-size.ts around the anchor and acc check so the merge-base remains
the stale boundary even when it has no Buildkite status; mark sizes resolved
from any older commit stale rather than enforcing them. Add a regression case in
test/internal/binary-size-baseline.test.ts covering a statusless merge-base and
an older over-threshold baseline that is reported stale without failing.
In `@test/internal/binary-size-baseline.test.ts`:
- Around line 62-65: Extend the metadata-fallback test fixtures around the
mBuild response and add a matrix covering each accepted release-marker spelling:
release, build release, and release build. Mark an older fallback build with
each marker, run the canary comparison, and assert that it skips that build in
favor of the canary baseline.
---
Outside diff comments:
In `@scripts/binary-size.ts`:
- Around line 55-56: Update the request construction in the binary-size flow to
URL-encode the baseBranch-derived sha query value before interpolating it into
the commits request. Apply encoding at the request sink while preserving the
existing baseBranch fallback and baseline-selection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c80c4092-d902-44bc-a9f4-bafe1219a0de
📒 Files selected for processing (2)
scripts/binary-size.tstest/internal/binary-size-baseline.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/binary-size.ts (1)
168-172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not silently fall back to
baseBranchwhen merge-base resolution fails.A failed compare request leaves
walkFromset tobaseBranch, reverting to the stale-main behavior this PR is intended to eliminate. Retry the request or fail with an explicit diagnostic instead of walkingmainwithout a verified merge base.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/binary-size.ts` around lines 168 - 172, Update the merge-base resolution in the commit/branch comparison block so a failed githubJson request does not leave walkFrom using baseBranch. Retry the compare request or terminate with an explicit diagnostic, and only assign walkFrom after obtaining a verified merge_base_commit.sha.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/binary-size.ts`:
- Around line 168-172: Update the merge-base resolution in the commit/branch
comparison block so a failed githubJson request does not leave walkFrom using
baseBranch. Retry the compare request or terminate with an explicit diagnostic,
and only assign walkFrom after obtaining a verified merge_base_commit.sha.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59f3693e-0a72-4cec-b44a-b8190825977f
📒 Files selected for processing (1)
scripts/binary-size.ts
|
Self-review complete. All four surviving concerns addressed:
The merge-base compare block is also load-bearing in the fixture as of 7103a52 (main HEAD is one commit ahead of the fixture merge-base). |
There was a problem hiding this comment.
LGTM — CI-tooling fix with all prior review findings addressed across 7103a52 and ee4719c.
What was reviewed:
- Anchor placement:
anchor ??= nnow sits after the like-for-like check but still fires on the!recordbranch, so a[release]merge-base with a successful artifact defers enforcement to the next canary build while a fully-canceled merge-base still marks older rows stale. - Meta-data fallback declines non-
webhookand[release]-tagged builds, so release-mode sizes are never enforced against a canary PR; both signals covered by the parametrized test. - Fixture now has
sha-headahead of the merge-base, making thecompare/walk load-bearing; stale-row<sup>link and empty-accno-baseline note both asserted.
Extended reasoning...
Overview
This PR touches only scripts/binary-size.ts (the Buildkite binary-size aggregator step) and adds test/internal/binary-size-baseline.test.ts. It fixes a CI false-positive where every PR was failing "12 over 0.50 MB" because a run of canceled main builds left the baseline stuck at #79916 while main grew ~550 KB. The fix walks from the PR's merge-base instead of main HEAD, resolves per-target baselines (with a per-triplet meta-data fallback when the aggregator artifact is missing), and marks rows whose baseline predates the merge-base as stale — annotated but never enforced.
Security risks
None. This is a CI annotation/gate script that reads Buildkite meta-data and public GitHub/Buildkite JSON, then writes an HTML annotation. It does not touch the runtime, ship in the binary, handle user input, or have any auth/crypto surface. The two new env-var overrides (BINARY_SIZE_GITHUB_API, BINARY_SIZE_BUILDKITE_WEB) are test-only injection points into a script that only runs inside Buildkite.
Level of scrutiny
Medium-low. The blast radius is bounded to a single Buildkite step that already carries a [skip size check] escape hatch and runs with --no-fail on main. The worst possible regression is either a false-positive (PR annotated red when it shouldn't be — the exact bug being fixed) or a false-negative on a narrow condition (PR branched from a fully-canceled or release-mode merge-base bypasses the gate), and the latter is still visible as
Other factors
This PR has been through two prior review rounds from me plus CodeRabbit, and every finding was addressed with a follow-up commit and a matching test:
- 92583dc: empty-
accguard so "no comparison" renders instead of "all within"; meta-data fallback recovers the release flag. - 7103a52:
anchor ??= ndeferred past the like-for-like check; fixture gainedsha-headso the merge-base compare block is load-bearing. - ee4719c: fallback declines non-
webhookbuilds (real Bun releases aresource:ui/RELEASE=1, not[release]-tagged commits); stale<sup>link asserted; both release signals covered by a parametrized test.
The four tests spawn the real script against a local mock GitHub/Buildkite server and a bash buildkite-agent shim, are hermetic (no network), skipIf(!isPosix)-gated, and assert exact stdout/annotation content plus exit codes. The bug hunting system found nothing this run. All review threads are resolved.
There was a problem hiding this comment.
LGTM — CI-tooling-only fix for the stale binary-size baseline false-positives; all four earlier findings from this reviewer are addressed and resolved.
What was reviewed:
- Anchor semantics after 7103a52: a like-for-like mismatch skips without claiming the anchor (next canary build enforced), while an unusable merge-base claims it so older rows go stale — matches the tests.
- Meta-data fallback after ee4719c: non-
webhookand[release]-tagged baselines are declined so release-mode sizes are never enforced against a canary PR; both signals covered by the parametrized test. - Empty-
accguard restores the "no comparison" note;walkFrom/baseBranchare URL-encoded; thesha-headfixture makes the merge-base compare block load-bearing.
Extended reasoning...
Overview
Changes are confined to scripts/binary-size.ts (the Buildkite binary-size annotation/gate step) and a new test/internal/binary-size-baseline.test.ts. No src/**, no user-facing runtime code. The script now (a) walks from the PR's merge-base via the GitHub compare API, (b) accumulates per-triplet baselines with a Buildkite meta-data fallback when the aggregator artifact is missing, and (c) marks rows whose baseline predates the merge-base as stale — annotated with buildkite-agent shim.
Security risks
None. The script runs only in CI, reads Buildkite/GitHub metadata, and emits an HTML annotation. New env knobs BINARY_SIZE_GITHUB_API / BINARY_SIZE_BUILDKITE_WEB are test-only overrides; baseBranch and walkFrom are encodeURIComponent-escaped before interpolation into API URLs.
Level of scrutiny
Low-medium. This is CI presentation/gating tooling, not runtime. Worst-case failure modes are a false-positive size gate (what this PR fixes) or a false-negative that lets a size regression annotate as "N stale ignored" without hard-failing — both recoverable, both visible in the annotation, and the [skip size check] escape hatch is unchanged. The design deliberately fails open (stale, not enforced) when the baseline's canary/release provenance can't be established, which is the documented and tested behaviour.
Other factors
Two review rounds from this reviewer raised four issues (empty-acc header regression; meta-data fallback lacking a release flag; anchor pinned before the like-for-like check; merge-base compare block not load-bearing in the fixture). All were fixed in 92583dc / 7103a52 / ee4719c and are marked resolved; CodeRabbit's three findings were either addressed or withdrawn. The added tests drain stdout/stderr/exit concurrently, use tempDir/bunEnv/port: 0, run sequentially against a shared mock (documented), and skipIf(!isPosix) for the bash shim. No CODEOWNERS entry covers scripts/. The most recent commit (25965f4) is a CI retrigger only.
|
CI on build 82347 verified the fix end-to-end: the binary-size step now reports "all within 0.50 MB" against The remaining red is Ready for a maintainer. |
|
This bug hit again after #41330 merged. That commit shrank the android and freebsd binaries by 12.7 MB. Every PR not yet rebased onto ae7b8f4 now fails the size check with +12.7 MB on those targets, for example build 111010 (branch based on 16eb85a, scripts-only change). PRs based on ae7b8f4, for example builds 111016, 111017 and 111018, pass. I reproduced it with the current script against the real data of build 111010 and a stub A minimal version of the merge-base part of this PR is on branch robobun/a1f232a8/binary-size-merge-base (commit 8da77ac, 19 lines in |
|
can we just change this to a warning? |
…whose baseline predates the merge-base The step was comparing every PR against the newest main build whose binary-size aggregator step had run. When a streak of main builds has any build-bun job time out or get canceled, Buildkite never runs the aggregator (allow_dependency_failure does not cover canceled/timed-out deps), so no binary-sizes.json is uploaded and the baseline falls back to an older main build. The delta then includes main's own growth since that older build, and every PR based on current main trips the 0.5 MB threshold. Fix: resolve each target's baseline independently. Walk main from the PR's merge-base; for each build, try the binary-sizes.json artifact first, then fall back to the per-target binary-size:<triplet> meta-data that the individual build-bun jobs already set. A target whose only available baseline is older than the merge-base is annotated (with the actual source build linked) but never fails the step.
… fallback; return no-baseline when walk finds no sizes
…se] merge-base does not disable enforcement; make the merge-base walk load-bearing in the test fixture
…real Bun releases are source:ui/RELEASE=1, not [release]-tagged); assert the stale-row <sup> build link; cover both release signals
The previous two commit subjects contained the literal bracketed release token, which .buildkite/ci.mjs interprets as a release-build trigger. That flipped the whole run to release mode: the --app feature (canary only) was disabled so test/bake/dev/production.test.ts failed on every lane, and the binary-size step ran with --release and skipped the comparison. This commit's subject is clean so the run is canary again.
The step annotates with a warning style and exits 0. Remove the --no-fail flag, the recordOnly plumbing in ci.mjs, and the [skip size check] commit-message escape hatch: there is nothing to skip any more.
25965f4 to
803fcf6
Compare
robobun
changed the title
|
@dylan-conway done in 803fcf6: the step now posts a warning-style annotation when a target grows past the threshold and always exits 0. Removed I kept the merge-base / per-target baseline logic so the warning itself is accurate (otherwise every PR would still show ~+550 KB whenever main's aggregator step skips a few builds). If you would rather have the minimal change only (warning on the old baseline walk), say so and I will strip it down. Also rebased onto current main (d316760); the PR title/body are updated. |
The test spawned a debug bun that spawned a bash agent shim ~17 times and made ~10 HTTP calls to a debug Bun.serve. Under ASAN that took 1.5 to 5 s per test and tripped the 5 s default timeout on a loaded machine. The CI logic now lives in run(), which takes the buildkite-agent and fetch calls through an Io object, so the test drives it directly with fakes (~5 ms per test). This also removes the BINARY_SIZE_GITHUB_API / BINARY_SIZE_BUILDKITE_WEB env hooks that existed only for the old test.
This branch has not been deployed
2 participants
Problem
12 over 0.50 MB(every target ~+550 KB), e.g. build 82300. Those PRs added on the order of 16 KB.*-build-bundep time out or get canceled, which Buildkite treats as not-a-failure forallow_dependency_failure, so the aggregator never ran and the baseline stayed at #79916 (pre-QUIC, node:quic on lsquic — Node v26 compat, HTTP/3 #32602).Fix
--no-failflag,recordOnlyplumbing, and[skip size check]escape hatch are removed.binary-size:<triplet>meta-data each*-build-bunjob sets even when the aggregator never ran. A target whose only baseline is older than the merge-base is listed with its source build and does not raise the warning, because that delta folds in main's own growth.[release]commit subject), so release-mode sizes are never compared against a canary PR.test/internal/binary-size-baseline.test.tsdrives the exportedrun()in-process with a fakebuildkite-agentand fake GitHub/Buildkite responses (the agent and HTTP calls are injected through anIoobject). Also build 82347 on the pre-warning version of this branch: "all within 0.50 MB" against merge-base #81770, 9 stale rows annotated.Background
scripts/binary-size.tsruns once per build after every*-build-bunjob. It reads each job's stripped size from Buildkite meta-data and compares against a baseline.source: "ui") triggers withRELEASE=1; their Windows binaries differ from canary by several MB, so the two must never be compared.Notes
Applied to build 82300 (PR #35901, merge-base = main HEAD at the time):
bun-darwin-x64was 66,565,564 in the PR vs 66,565,548 in main #81770's meta-data (+16 B), but against #79916 (66,008,012) it read +544.5 KB.Review history: two rounds each from claude[bot] and coderabbit, plus a self-review. Findings addressed: empty-baseline header, anchor pinned before the release-kind filter, meta-data fallback accepting release sizes, merge-base walk not load-bearing in the fixture, stale-row link not asserted. Withdrawn (with reasons in-thread): fetch timeouts, absolute temp dir,
parseIntstrictness, zero-as-absent checks.Rebased onto current main (d316760) when switching to warning mode;
scripts/binary-size.tshad no intervening changes.[auto-merge] gate passed · iteration 5 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 5
evidence per changed file
root cause · written by the author bot
The binary-size check compared every PR build against a fixed, stale canary reference (main #79916) that had stopped advancing, so the roughly 550KB that main itself had grown since that build was attributed to each unrelated PR and tripped the 0.50 MB threshold. The fix makes
scripts/binary-size.tsresolve the baseline from the PR's merge-base main build via the GitHub and Buildkite APIs, with artifact and per-triplet metadata fallback, so each PR is measured only against its own contribution. When a triplet's baseline is stale or unavailable, the comparison is reported as a warning with…