Repository navigation
ci: fix the Windows sign step on the Build Tools image - #46462
Conversation
|
Updated 1:05 AM PT - Oct 22nd, 2026
@dylan-conway, your commit 350558b is building: |
✨ Finishing Touches: 💡 1
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 |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked exit-code propagation through the new wrapper (scripts/vs-shell.ps1 ends with exit $LASTEXITCODE, so the sign script's explicit exit 0/exit 1 reaches Buildkite) and the dropped -ExecutionPolicy Bypass (the test and profile steps at .buildkite/ci.ts:856 and :939 already run vs-shell.ps1 under pwsh -NoProfile -File without it) — neither is a problem.
Extended reasoning...
The change moves the Windows release-signing step from powershell -File sign-windows-artifacts.ps1 (which dot-sourced vs-shell.ps1) to pwsh -File vs-shell.ps1 pwsh -File sign-windows-artifacts.ps1, and adds fail-fast throws for a missing VSINSTALLDIR and a missing signtool.exe. It touches the path that fetches DigiCert signing secrets and signs release binaries, and the author states the scripts were not run locally, so the interpreter switch (5.1 to 7) deserves a human look alongside the inline finding; the changed paths have no CODEOWNERS entry.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
.buildkite/scripts/sign-windows-artifacts.ps1— Release operators can get signing credentials silently corrupted instead of a clear failure when buildkite-agent prints anything to stderr. Under powershell 5.1 the2>&1at .buildkite/scripts/sign-windows-artifacts.ps1:52 with Stop turned the first stderr line into a terminating error. Under pwsh 7 (the new launcher at .buildkite/ci.ts:1067) the stderr line is instead merged into $value as an extra element, the check at :53 passes, and $env:SM_API_KEY at :61 becomes the secret joined with the warning text. Fix: capture stdout only (drop 2>&1 or redirect stderr separately) and trim/validate the single returned line before assigning it to the SM_* env vars.Why this was flagged
If buildkite-agent ever writes a warning or notice line to stderr during
secret get, the new behaviour differs from the base. On the base commit the step ran under Windows PowerShell 5.1, where& buildkite-agent secret get $Name 2>&1at .buildkite/scripts/sign-windows-artifacts.ps1:52 with $ErrorActionPreference = "Stop" (:22) terminates on the first stderr line and the catch at :281 prints that text. Under pwsh 7.5.2, launched by .buildkite/ci.ts:1067, native stderr no longer respects Stop, so $value becomes an array of the secret plus an ErrorRecord.[string]::IsNullOrEmpty($value)at :53 stringifies the array and passes. The function returns the array, and the assignment to $env:SM_API_KEY (:61) or $env:SM_CLIENT_CERT_FILE (:63) joins the elements with a space. The certificate path fails later in Setup-Certificate with "SM_CLIENT_CERT_FILE is neither valid base64 nor an existing file" (:86), or the API key fails inside smctl with an opaque auth error. Remedy: capture only stdout for the secret and validate it is a single non-empty line.Verification: Triggers when
buildkite-agent secret getwrites any line to stderr yet exits 0. The diff switches the launcher frompowershelltopwshat /home/claude/bun/.buildkite/ci.ts:1067. In PowerShell 7 native stderr no longer honors ErrorActionPreference, so the stderr line is merged into$valueat /home/claude/bun/.buildkite/scripts/sign-windows-artifacts.ps1:52; the check at :53 passes and$env:SM_API_KEYat :61 stringifies the array.
What does this PR do?
The Windows sign step failed on the v1.4.4 release build (130733) with
exec: "signtool": executable file not found in %PATH%.smctlrunssigntool.exe, which is only in PATH oncevs-shell.ps1has loaded the Visual Studio environment, and that load failed without stopping the script.The sign step was the only caller that dot-sourced
vs-shell.ps1from another script underpowershell(5.1). Run that way, the first lookup in Microsoft'sLaunch-VsDevShell.ps1("using the script path") fails. It also failed on the v1.4.3 release build (124221), where the script's fallback ("the latest Visual Studio installation") then found VS Community. Since #45144 the image has Build Tools, and on 130733 that fallback found no installation. The step only runs on release builds, so this is its first run on the new image.pwsh -NoProfile -File .\scripts\vs-shell.ps1 <command>, like the test and trace-order steps. On 130733 those two loaded Build Tools 17.14.41 at the first lookup, on the same image.vs-shell.ps1throws whenVSINSTALLDIRis still unset afterLaunch-VsDevShell.ps1returns, so a failed load stops the step there.signtool.exebefore it fetches secrets or downloads anything.None of these files is a bake input, so no image is baked again.
How did you verify your code works?
The head commit's message has the tag that makes
ci.tssign on a branch, so its build runs the sign step on a Windows agent. That signs four executables with DigiCert. It is the only check of the fix: the scripts were not run locally.Not determined: which of the two differences (
powershellinstead ofpwsh, or the dot-sourcing) makes the first lookup fail. This removes both.