Skip to content

ci: sign on an image bake, and read option tags from the subject only #46464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dylan-conway wants to merge 1 commit into
base: main
Choose a base branch
from
+24 −11
Open

ci: sign on an image bake, and read option tags from the subject only #46464

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions .buildkite/ci.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1448,6 +1448,16 @@ function getSelectedPlatform(platforms: Map<string, Platform>, key: string): Pla
return platform;
}

/**
* What option tags like [skip tests] are read from. On main the lines below
* the subject are a pull request's description, which can mention a tag
* without asking for it. A pull request's first build has the pull request's
* title here, not the head commit's subject.
*/
function getCommitSubject(): string | undefined {
return getCommitMessage()?.split("\n", 1)[0];

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (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.

}

async function getPipelineOptions(): Promise<PipelineOptions | undefined> {
const isManual = isBuildManual();
if (isManual && !process.argv.includes("--apply")) {
Expand Down Expand Up @@ -1506,15 +1516,13 @@ async function getPipelineOptions(): Promise<PipelineOptions | undefined> {
};
}

// BUILDKITE_MESSAGE is the commit subject line only — option tags like
// [skip tests] must appear in the subject, not the commit body.
const commitMessage = getCommitMessage();
if (commitMessage === undefined) {
const subject = getCommitSubject();
if (subject === undefined) {
throw new Error("Failed to read the commit message");
}

const parseOption = (pattern: RegExp): OptionFlag => {
const match = pattern.exec(commitMessage);
const match = pattern.exec(subject);
if (match) {
const [, value] = match;
return value;
Expand All @@ -1523,7 +1531,7 @@ async function getPipelineOptions(): Promise<PipelineOptions | undefined> {
};

const isCanary =
!parseBoolean(process.env.RELEASE || "false") && !/\[(release|build release|release build)\]/i.test(commitMessage);
!parseBoolean(process.env.RELEASE || "false") && !/\[(release|build release|release build)\]/i.test(subject);

return {
canary: isCanary ? canary : 0,
Expand Down Expand Up @@ -1653,10 +1661,6 @@ async function getPipeline(options: PipelineOptions = {}): Promise<Pipeline | un
}

const { buildPlatforms = [], testPlatforms = [] } = options;
// Sign with [sign windows] in the commit message (for testing the sign step
// on a branch). DigiCert charges per signature, so canary builds are never signed.
const windowsPlatforms = buildPlatforms.filter(p => p.os === "windows");
const signWindows = ((isMainBranch() && !options.canary) || !!options.signWindows) && windowsPlatforms.length > 0;

// A build makes sure every image of the spec exists, not only the ones its
// own steps start from: which steps a build has depends on its options (a
Expand Down Expand Up @@ -1711,6 +1715,15 @@ async function getPipeline(options: PipelineOptions = {}): Promise<Pipeline | un
}
}

// DigiCert charges per signature, so canary builds are not signed. That
// leaves a release as the first build to find out that a new image broke the
// 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));

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

const signWindows =
windowsPlatforms.length > 0 && ((isMainBranch() && !options.canary) || !!options.signWindows || bakesSignImage);
Comment on lines +1722 to +1725

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Comment on lines +1723 to +1725

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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.


const includeASAN = !isMainBranch();

// verify-baseline / trace-order: checks that run on a built binary on a
Expand Down Expand Up @@ -1819,7 +1832,7 @@ async function getPipeline(options: PipelineOptions = {}): Promise<Pipeline | un
{ os: "darwin", arch: "x64", release: "any" },
];
const darwinTestsEnabled =
isMainBranch() || isBuildManual() || /\[(macos|darwin) tests?\]/i.test(getCommitMessage() ?? "");
isMainBranch() || isBuildManual() || /\[(macos|darwin) tests?\]/i.test(getCommitSubject() ?? "");
// The macOS beta lane: a single home-hosted mini on the next macOS,
// its own queue, soft-fail. PR builds get it only when that queue is
// idle at upload time, so it is always busy while PRs flow and never
Expand Down
Loading