ci(windows): stash multi-line env vars around Enter-VsDevShell

[robobun]

Problem

On Windows CI lanes only, every test that spawns a Bun subprocess via bunEnv failed at startup with:

error: invalid JSC environment variable

    BUN_JSC_collectContinuously=1 stress over serve+ws+reload and over

That string is a line from the commit message body. It does not appear in any code. Example: build 73847 (PR #34346), where every Windows test lane went red with the same error; Linux/macOS lanes were unaffected. It was worked around by amending the commit message, but any future commit whose body has a line starting BUN_JSC_*= (or any other KEY=value that a downstream tool inspects) will hit it again.

Cause

Windows test steps are wrapped in scripts/vs-shell.ps1, which calls Enter-VsDevShell from Microsoft.VisualStudio.DevShell.dll to load the MSVC toolchain. That cmdlet captures the environment by running VsDevCmd.bat inside cmd.exe and parsing the output of the set command line by line. set has no way to represent a value that contains CR/LF: each physical output line is treated as an independent KEY=VALUE entry.

Buildkite exports the full multi-line commit message as BUILDKITE_MESSAGE. Given

BUILDKITE_MESSAGE=commit subject
<blank>
BUN_JSC_collectContinuously=1 stress over serve+ws+reload and over
some more text

Enter-VsDevShell writes back BUILDKITE_MESSAGE=commit subject, promotes the third line to a brand-new BUN_JSC_collectContinuously env var with value 1 stress over serve+ws+reload and over, and echoes the non-= line to stdout. That env var then flows through node scripts/runner.node.mjs into every spawned bun process, where JSCInitialize rejects it (the value is not a valid boolean) and exits 1.

This cannot be fixed downstream in test/harness.ts or runner.node.mjs: by the time they run, the spurious var already exists under a name that does not start with BUILDKITE, so there is no way to tell it apart from a deliberately set JSC option.

Fix

Before invoking Launch-VsDevShell.ps1, stash every environment variable whose value contains CR or LF into a hashtable and remove it from env:. Restore them in finally once the loader returns. The VS dev shell only ever sees single-line input, and the wrapped command sees the original multi-line values intact.

Verification

Reproduced and verified on a Windows 2019 host with VS 2022 17.14. With

$env:BUILDKITE_MESSAGE = "commit subject`n`nBUN_JSC_collectContinuously=1 stress over serve+ws+reload and over`nsome more text"
pwsh -NoProfile -File .\scripts\vs-shell.ps1 bun -e 'console.log(JSON.stringify({ bk: process.env.BUILDKITE_MESSAGE, jsc: process.env.BUN_JSC_collectContinuously }))'

Before

some more text
$ bun -e ...
error: invalid JSC environment variable

    BUN_JSC_collectContinuously=1 stress over serve+ws+reload and over

(exit 1; some more text is the line without =, echoed by the DevShell parser)

After

$ bun -e ...
{"bk":"commit subject\n\nBUN_JSC_collectContinuously=1 stress over serve+ws+reload and over\nsome more text","jsc":null}

(exit 0; BUILDKITE_MESSAGE preserved verbatim, no spurious env var)

Also checked: VSINSTALLDIR/VCToolsVersion/INCLUDE/LIB are still set and cl.exe is on PATH after the change; CRLF values are handled; with no multi-line vars present the stash loop is a no-op.

test/internal/vs-shell-multiline-env.test.ts is the automated guard: Windows-only (skipped when vswhere.exe is absent), it spawns a fresh pwsh with VSINSTALLDIR unset so Enter-VsDevShell runs inside the test, with a probe value whose body contains both a KEY=value line and a BUN_JSC_*=... line. It fails on main with the exact invalid JSC environment variable error and passes with this change.

The fix lives entirely under scripts/, so the fail-before gate's git stash push -- src/ packages/ cannot demonstrate it (stashing src/ leaves the script change in place). The before/after above and the new test are the proof.

---

no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5e9b3090-82a2-4acd-b871-040b0b9d4af0

📥 Commits

Reviewing files that changed from the base of the PR and between 0ecd508 and 9456e26.

📒 Files selected for processing (2)

• scripts/vs-shell.ps1
• test/internal/vs-shell-multiline-env.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 7:30 AM PT - Jul 16th, 2026}

❌ @robobun, your commit 9456e26 has 1 failures in Build #73893 (All Failures):

• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 34355

That installs a local version of the PR into your bun-34355 executable, so you can run:

bun-34355 --bun

[claude]

LGTM — small, self-contained Windows CI script fix with a clear root cause and verified before/after.

What was reviewed:

• Stash/restore is symmetric: $multilineEnv is always initialized before the try, and restore runs in finally on both success and throw paths.
• -LiteralPath on Remove-Item/Set-Item avoids wildcard interpretation for names like ProgramFiles(x86).
• Get-ChildItem env: is enumerated as a snapshot, so removing entries inside the loop is safe; [\r\n] correctly catches both CR and LF.

Extended reasoning...

Overview

This PR touches a single file, scripts/vs-shell.ps1, which wraps Windows CI commands in the Visual Studio developer shell. It adds ~14 lines that stash any environment variables containing CR/LF into a hashtable, remove them before invoking Launch-VsDevShell.ps1, and restore them in the existing finally block. The root cause (Microsoft's Enter-VsDevShell parses cmd /c set output line-by-line and promotes lines from multi-line values like BUILDKITE_MESSAGE into new env vars) is well-documented in the PR description with a concrete failing build and a manual before/after repro on Windows.

Security risks

None. This is CI-only PowerShell that manipulates the process's own environment variables; no new inputs are parsed, no external commands are added, and the values being stashed/restored are already present in the environment. No src/ or test-harness code is touched.

Level of scrutiny

Low. This is build/CI tooling, not shipped runtime code, and the change is additive around an existing try/finally. The failure mode of a bug here would be a visibly broken Windows CI lane, not a silent runtime regression. There is no CODEOWNERS entry for scripts/.

Other factors

• Checked that $multilineEnv is defined unconditionally before the try block, so the finally restore loop cannot see an undefined variable.
• -LiteralPath is used for both Remove-Item and Set-Item, which correctly handles env var names containing (/) or other glob characters.
• foreach over (Get-ChildItem env:) iterates a materialized snapshot, so Remove-Item inside the loop does not mutate the collection being walked.
• VS toolchain vars (PATH, INCLUDE, LIB, etc.) are single-line semicolon-delimited and won't be stashed; the PR author confirmed they remain set after the change.
• No prior reviews from me on this PR; only a CodeRabbit rate-limit notice in the timeline. The lack of an automated test is justified in the description (no harness drives vs-shell.ps1, and Enter-VsDevShell isn't available on Linux runners), consistent with prior CI-script PRs like #32377.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 test/internal/vs-shell-multiline-env.test.ts:70 — Nit: test/CLAUDE.md:120 says "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." — the CI runner already passes --timeout at the CLI level, so the trailing 60_000 argument here is redundant and violates that convention. Drop the 60_000, argument.

  Extended reasoning...

  What the issue is

  test/CLAUDE.md line 120 states:

  CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

  The new test file passes 60_000 as the third argument to test.skipIf(...)(name, fn, 60_000) at test/internal/vs-shell-multiline-env.test.ts:70, which sets an explicit per-test timeout. This is a newly-introduced violation of an explicit repository instruction.

  Why the rule exists

  The CI test runner (scripts/runner.node.mjs) already passes --timeout=${perTestTimeout} at the CLI level, which is what "Bun already has timeouts" refers to. Per-test timeouts scattered across individual test files make it harder to tune timeout behavior centrally (e.g., when debug/ASAN builds need longer timeouts across the board), and they tend to accumulate as copy-paste artifacts. No other test in test/internal/ sets an explicit timeout.

  Step-by-step

  1. test/CLAUDE.md:120 — the rule is explicit and marked CRITICAL.
  2. test/internal/vs-shell-multiline-env.test.ts:70 — the trailing 60_000, is the third positional arg to the test.skipIf(...)(...) call, which bun:test interprets as the per-test timeout in ms.
  3. The test spawns pwsh which invokes Enter-VsDevShell — this can legitimately be slow, which is presumably the motivation, but the CI runner's CLI-level --timeout already accounts for slow subprocess tests on Windows lanes.

  Impact

  None functionally — the test works either way. This is purely a repository-convention violation. It will not cause a concrete failure if merged as-is; the CLI-level timeout and the per-test timeout coexist without conflict. Marking as nit because merging without the fix causes no runtime breakage.

  How to fix

  Delete the trailing 60_000, argument on line 70:

       expect(exitCode).toBe(0);
     },
  -  60_000,
   );

  If Enter-VsDevShell genuinely needs more than the default CI timeout on some Windows hosts, the right lever is the CI runner's per-file timeout configuration, not a per-test override.

[robobun]

Dropped the per-test timeout in 9456e26. The CI runner already passes --timeout=90000 (or higher under ASAN), and in practice the inner Enter-VsDevShell is pre-warmed: on CI the test runner itself is wrapped in vs-shell.ps1, and locally bun bd re-execs through it (scripts/build.ts), so the test body sees ~1.5s.

[claude]

LGTM — the timeout nit was addressed in 9456e26.

What was reviewed:

• Stash/restore in vs-shell.ps1: -LiteralPath used for both remove and set, restore is in finally so it runs even if Launch-VsDevShell.ps1 throws, and $multilineEnv is initialized before try so the finally never sees an unset hashtable.
• Considered whether hiding a multi-line var from the VS loader could break toolchain setup — no, VS env vars are single-line, and any multi-line one was already corrupted by the set parsing this works around.
• Test: forces the loader path by unsetting VSINSTALLDIR, drains pipes concurrently, asserts exact round-trip + no leaked vars + VS env still loaded, exitCode checked last.

Extended reasoning...

Overview

Two files: scripts/vs-shell.ps1 gains a ~14-line stash/restore around the Launch-VsDevShell.ps1 invocation, and test/internal/vs-shell-multiline-env.test.ts is a new Windows-only regression guard. No runtime (src/) code is touched — this is purely CI/build infrastructure for the Windows lanes.

Security risks

None. The script only reads and rewrites its own process environment; nothing from untrusted input is interpreted or executed. -LiteralPath is used for both Remove-Item and Set-Item, so env var names containing wildcard characters aren't glob-expanded.

Level of scrutiny

Low-to-medium. This is a Windows CI wrapper script, not shipped runtime code. The failure mode of a mistake here is a red Windows CI lane, not a user-facing bug. The change is mechanically simple (hashtable stash → remove → restore in finally) and the PR description includes a manual before/after repro on a real Windows host.

Other factors

• The root-cause analysis is precise (Enter-VsDevShell parses cmd /c set line-by-line; BUILDKITE_MESSAGE is multi-line) and the fix targets exactly that layer — the only place that can distinguish the spurious var from a real one.
• $multilineEnv = @{} is assigned before the try, so the finally restore loop is safe even when zero vars were stashed or the loader threw immediately.
• Get-ChildItem env: is enumerated into the foreach before mutation, so removing items mid-loop is safe.
• Hiding multi-line vars from the VS loader is strictly no worse than before: any such value was already being corrupted by the set parser, and no MSVC toolchain variable is multi-line.
• The test follows harness conventions (bunEnv/bunExe/isWindows, Promise.all on stdout/stderr/exited, structured toEqual, exitCode asserted last, skipIf gated on vswhere.exe). It deletes VSINSTALLDIR from the child env so the stash path actually executes even though the test runner itself already sits inside a VS shell.
• My only prior comment (drop the explicit 60_000 timeout) was addressed and the thread is resolved.

[robobun]

CI (build 73893): the only red is test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js on debian 13 x64-asan, which is a pre-existing failure on main (unrelated to this diff). All Windows test lanes passed, including the new test/internal/vs-shell-multiline-env.test.ts. The remaining macOS lanes are still queued; this change is a no-op there (the test is test.skipIf(!isWindows) and scripts/vs-shell.ps1 is Windows-only).

Ready for review.

[robobun]

Another instance of this, for the record: build 74278 (PR #31951) has test/js/node/v8/capture-stack-trace.test.js red on all three Windows lanes. The commit body there contains BUN_DESTRUCT_VM_ON_EXIT=1 unless the file is listed in, which Enter-VsDevShell promotes to a real env var. With that set, the test fixture's process.exit(0) runs close_all_socket_groups during global_exit, which fires the WebSocket close listener, which calls process.exit(1), so the fixture exits 1 with stdout hi 0.

Verified locally on Windows 2019 with the build 74278 profile binary: fails through the current scripts/vs-shell.ps1, passes through this branch's version. The runner's env dump in the job log shows the leaked var directly:

BUN_DESTRUCT_VM_ON_EXIT: 1 unless the file is listed in