Repository navigation
ci: sign on an image bake, and read option tags from the subject only - #46464
dylan-conway wants to merge 1 commit into
Conversation
…ild bakes the sign step's image
|
Updated 6:23 AM PT - Oct 22nd, 2026
❌ @dylan-conway, your commit 3ce63cc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46464That installs a local version of the PR into your bun-46464 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughBuildkite CI now reads commit tags from the first line of the commit message. Windows signing is also enabled when a build is baking the signing image and builds are not skipped. ChangesCommit subject tag checks
Windows signing conditions
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes both main changes: signing during an image bake and reading option tags from the commit subject only. |
| Description check | |
The description follows the required template. It explains the changes and documents verification results, including the tests that were not run. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const windowsPlatforms = buildPlatforms.filter(p => p.os === "windows"); | ||
| const bakesSignImage = !buildId && baking.has(getImageKey(windowsSignPlatform)); | ||
| const signWindows = | ||
| windowsPlatforms.length > 0 && ((isMainBranch() && !options.canary) || !!options.signWindows || bakesSignImage); |
There was a problem hiding this comment.
🔴 A manual build on an in-repo branch that bakes the windows-x64 image now gets a failing windows-sign step whenever a non-release profile was picked for Windows. bakesSignImage at .buildkite/ci.ts:1723 turns signing on for manual builds, which base never signed on branches, and windowsPlatforms at .buildkite/ci.ts:1722 keeps debug, assert and asan targets. The sign step then asks those build steps for <triplet>-profile.zip and <triplet>.zip, which only a plain release build uploads, and the script throws on the missing download. Fix: only sign targets whose profile is release ((p.profile ?? "release") === "release"), at the one filter the sign step and release step share, which also fixes the pre-existing main manual case.
Why this was flagged
A maintainer edits a bake input, pushes to oven-sh/bun, and starts a manual build picking a Windows platform with profile debug, assert or asan (.buildkite/ci.ts:1506-1509 expands every picked platform by every picked profile). The windows-x64 2019 image is missing or pending, so the loop at .buildkite/ci.ts:1684-1697 adds its key to baking, and bakesSignImage at .buildkite/ci.ts:1723 is true; windowsPlatforms at .buildkite/ci.ts:1722 keeps the target. getWindowsSignStep at .buildkite/ci.ts:1047-1052 lists bun-windows-x64-profile.zip and bun-windows-x64.zip from step windows-x64-debug-build-bun. packageAndUpload in scripts/build/ci.ts:326-358 names the variant zip after the exe (bun-windows-x64-debug, not -profile) and only uploads the stripped ${bunTriplet}.zip when shouldStrip (plain release). Download-Artifact in .buildkite/scripts/sign-windows-artifacts.ps1:170-173 throws Failed to download artifact, failing the step and the build. On the base branch the manual path returns no signWindows (.buildkite/ci.ts:1500-1516) and a branch build never signed, so this build was green. The windowsPlatforms.length > 0 guard does not help: it only checks that some Windows target exists, not its profile.
Verification: bakesSignImage at .buildkite/ci.ts:1723 is true, and windowsPlatforms at 1722 keeps the debug/assert/asan targets, so signWindows at 1724-1725 is true. getWindowsSignStep (1043-1072) then asks windows-x64-debug-build-bun for bun-windows-x64-profile.zip and bun-windows-x64.zip. But that build step only uploads bun-windows-x64-debug.zip (scripts/build/ci.ts:326) and no stripped zip. On base the branch route did not exist.
| // sign step, so a build that bakes the step's image signs what it builds. | ||
| // [sign windows] is for testing the step on a branch. | ||
| const windowsPlatforms = buildPlatforms.filter(p => p.os === "windows"); | ||
| const bakesSignImage = !buildId && baking.has(getImageKey(windowsSignPlatform)); |
There was a problem hiding this comment.
🟡 nit (optional): Builds that only wait on another build's bake of the Windows sign image also sign, paying DigiCert for signatures the bake rationale does not need. baking at .buildkite/ci.ts:1695 gets the key for a "pending" image too, so bakesSignImage at .buildkite/ci.ts:1723 is true for every build that merely waits on the sign image. Fix: make the trigger true only when this build itself bakes the image (track keys whose state was not "pending" separately, or record the state per key) so waiting canary builds keep not signing. The comment at .buildkite/ci.ts:1718 and the variable name say "bakes"; the code says "bakes or waits".
Why this was flagged
A PR that changes a Windows bake input is pushed again, or merged, while its first build's bake of the windows-x64 image is still running. The upload job's loop at .buildkite/ci.ts:1684 asks getImageState, gets "pending", skips the artifact upload at .buildkite/ci.ts:1693 but still runs baking.add(key) at .buildkite/ci.ts:1695 and emits a wait step. bakesSignImage at .buildkite/ci.ts:1723 then reads baking.has(getImageKey(windowsSignPlatform)) as true for that waiting build, so signWindows at .buildkite/ci.ts:1725 is true and a windows-sign step is added at .buildkite/ci.ts:1889 even though this is a canary build that bakes nothing. On the base branch a canary build never signs; after the merge each build that waits on a pending sign-image bake signs all Windows artifacts, one DigiCert charge per signature. The bake window is up to bakeTimeout = 3 * 60 minutes at .buildkite/ci.ts:979, so several waiting builds can fall inside it.
Verification: nit. A build whose upload job runs while another build's bake of the windows-x64-2019 image is still in progress. .buildkite/ci.ts:1689-1696 only continues on "available"; for "pending" it skips the artifact upload but still runs baking.add(key) (line 1695), so this build bakes nothing. Line 1723 bakesSignImage is true for the waiting build, and line 1725 makes signWindows true.
| * title here, not the head commit's subject. | ||
| */ | ||
| function getCommitSubject(): string | undefined { | ||
| return getCommitMessage()?.split("\n", 1)[0]; |
There was a problem hiding this comment.
🟡 nit (optional): after this change the binary-size annotation still tells a contributor to add [skip size check] to "the commit message", but only the subject line is read now, so a tag added to the PR description or commit body no longer works. Fix: update scripts/binary-size.ts:206 and the header comment at scripts/binary-size.ts:13 to say the tag must be in the commit subject (PR title), so the remedy the failing check prints still works.
Why this was flagged
getCommitSubject at .buildkite/ci.ts:1458 keeps only the first line, and parseOption at .buildkite/ci.ts:1542 now matches [skip size check] against that line only. The Buildkite annotation emitted at scripts/binary-size.ts:206 when the size check fails says to add the tag to the commit message, and on main the body is the squashed PR description, which is where a contributor following that text would put it. On the base branch the tag in the body turned the option on (builds 94142, 100378 in the PR text show this); after merging it does not, so the contributor's next build fails the size check again with no hint why. The diff does not touch scripts/binary-size.ts, so nothing corrects the stale instruction.
Verification: nit. Trigger: a PR build whose binary-size step fails and whose author follows the printed remedy by putting [skip size check] in the body of the next commit rather than its subject. .buildkite/ci.ts:1458 keeps only the first line, so on the base branch a tag in the body did set skipSizeCheck; after this change it does not. The remedy text at scripts/binary-size.ts:206 was not updated.
| * title here, not the head commit's subject. | ||
| */ | ||
| function getCommitSubject(): string | undefined { | ||
| return getCommitMessage()?.split("\n", 1)[0]; |
There was a problem hiding this comment.
🟡 (optional) Builds whose BUILDKITE_MESSAGE begins with a newline or whitespace line silently lose every option tag, where the base branch honored them. getCommitMessage returns BUILDKITE_MESSAGE untrimmed (scripts/buildkite.ts:146-148), and getCommitSubject at .buildkite/ci.ts:1458 takes the text before the first "\n", which is "" for a leading-newline message. "" passes the undefined guard at .buildkite/ci.ts:1520, so parseOption at .buildkite/ci.ts:1525 and the [release] test at .buildkite/ci.ts:1534 match nothing; on the base branch the regexes ran over the whole message so a leading blank line did not matter. Fix: take the first non-empty line, e.g. trim the message before splitting, so leading whitespace or CR cannot empty the subject.
Why this was flagged
Trigger: any Buildkite build whose BUILDKITE_MESSAGE starts with "\n" or a whitespace-only line (a message set by an API trigger or a tool-written commit message); only the git log fallback at scripts/buildkite.ts:152-155 trims, the Buildkite env path at scripts/buildkite.ts:146-148 returns the raw string. getCommitSubject (.buildkite/ci.ts:1458) then returns "", which is not undefined, so the throw at .buildkite/ci.ts:1521 does not fire; instead every parseOption call at .buildkite/ci.ts:1525 returns false, isCanary at .buildkite/ci.ts:1534 is true, and darwinTestsEnabled at .buildkite/ci.ts:1835 ignores a [macos tests] tag. The user asked for [skip ci], [release], [sign windows] or [skip size check] and gets a full canary build with none of them, with no error. On the base branch the same regexes ran over the whole untrimmed message and matched the tag anywhere, so this input class worked. Remedy: derive the subject from the trimmed message, or the first non-blank line.
Verification: getCommitMessage() returns process.env.BUILDKITE_MESSAGE as-is (scripts/buildkite.ts:146-148). The new helper at .buildkite/ci.ts:1458 yields "" for a message starting with "\n". The guard at .buildkite/ci.ts:1520 is if (subject === undefined) so an empty string passes, and every parseOption at :1525 returns false. On the base branch the regexes ran over the whole message; after this change the tag is silently dropped.
| const bakesSignImage = !buildId && baking.has(getImageKey(windowsSignPlatform)); | ||
| const signWindows = | ||
| windowsPlatforms.length > 0 && ((isMainBranch() && !options.canary) || !!options.signWindows || bakesSignImage); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing: a build that reuses another build's binaries via [skip builds] still gets a windows-sign step when it is a main release or carries [sign windows], and that step depends on build-bun steps the pipeline does not emit. The new !buildId guard at .buildkite/ci.ts:1723 only covers the bake trigger; the other two conjuncts at .buildkite/ci.ts:1725 are not gated, although the block was moved below buildId precisely so it could be. Fix: gate the whole signWindows expression on !buildId so no sign step (and no signed release dependency) is generated when there are no freshly built Windows artifacts, which covers all three triggers.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A main build whose subject carries [skip builds] with RELEASE set or [release], or any branch build whose subject carries both [sign windows] and [skip builds], reaches getPipeline with options.skipBuilds set and buildId assigned at .buildkite/ci.ts:1712. Build steps are only emitted when !buildId (.buildkite/ci.ts:1766), yet signWindows at .buildkite/ci.ts:1725 is still true, so .buildkite/ci.ts:1889 pushes getWindowsSignStep, whose depends_on at .buildkite/ci.ts:1059 names <target>-build-bun keys that do not exist in this pipeline. The pipeline is either rejected at upload for an unknown dependency or the sign step runs and sign-windows-artifacts.ps1 fails to download artifacts from build steps that never ran, so the release never ships. The base branch behaves the same, so this is pre-existing; the diff's !buildId at .buildkite/ci.ts:1723 looks as if it covers skip-builds but applies only to bakesSignImage.
Verification: A [skip builds] build that is a main non-canary build or a branch build with [sign windows] sets buildId (.buildkite/ci.ts:1707-1716). Line 1723 gates only the new bake trigger; the first two disjuncts at line 1725 are not gated on buildId. Line 1766 wraps the only place build-bun steps are pushed, yet line 1889 pushes getWindowsSignStep, whose depends_on at line 1059 names those absent steps.
What does this PR do?
Two fixes to
.buildkite/ci.tsthat came out of the Windows sign step failing on a release build (#46462). Tags are written without their square brackets here.A build that bakes the sign step's image also signs. The sign step only ran on release builds, because DigiCert charges per signature. #45144 changed the Windows image in a way that broke the step, and nothing ran it until the next release a week later. Now the build that bakes
windows-x64signs the Windows binaries it builds, so the pull request that changes the image finds out. Once the image exists, later builds do not sign. A build that reuses another build's binaries (skip builds) has nothing to sign and does not.Option tags are read from the subject line only. A comment said
BUILDKITE_MESSAGEis the subject line only. It is the whole commit message, and on main the lines below the subject are the pull request's description. A description that only mentions a tag therefore turned it on:skip size checkskipSizeCheck: 'skip size check'skip size checkskipSizeCheck: 'skip size check'sign windowssignWindows: 'sign windows'The same goes for
skip ciandrelease.getCommitSubject()is now what both places that look for tags read.How did you verify your code works?
bun .buildkite/ci.tsoutside Buildkite readsgit log -1. With HEAD at ci: fix the Windows sign step on the Build Tools image #46462's merge commit, whose description mentionssign windows, main'sci.tsprintssignWindows: "sign windows"and generates awindows-signstep. This branch printssignWindows: falseand generates none.prettier --checkpasses.tsc -p scripts/tsconfig.jsonreports nothing inci.tsexcept the missing node types of a checkout withoutnode_modules.Not run: a tag in the subject, and the bake condition. Whether an image exists is only asked on Buildkite, and this PR changes no bake input, so its own build bakes nothing.