ci: unregress tart image gen for macos - #25134
Conversation
|
Updated 11:02 PM PT - Nov 26th, 2025
❌ @nektro, your commit 85c93db has 3 failures in
🧪 To try this PR locally: bunx bun-pr 25134That installs a local version of the PR into your bun-25134 --bun |
This comment was marked as duplicate.
This comment was marked as duplicate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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 (2)
scripts/bootstrap.sh (1)
315-318: Invalid shell operator~- this will cause a syntax error.The
~operator is not valid POSIX shell syntax. The[(test) command doesn't support regex matching. This will fail on POSIX-compliant shells.Use
casefor pattern matching, which is POSIX-compliant:- if [ "$alpine" ~ "_" ]; then - release="$(print "$alpine" | cut -d_ -f1)-edge" + case "$alpine" in + *_*) + release="$(print "$alpine" | cut -d_ -f1)-edge" + ;; + *) + release="$alpine" + ;; + esac + if false; then + : # placeholder to maintain structureOr more cleanly:
- if [ "$alpine" ~ "_" ]; then - release="$(print "$alpine" | cut -d_ -f1)-edge" - else - release="$alpine" - fi + case "$alpine" in + *_*) + release="$(print "$alpine" | cut -d_ -f1)-edge" + ;; + *) + release="$alpine" + ;; + esacscripts/utils.mjs (1)
2240-2272: Timeout handler is missing — connection can hang indefinitelyThe review comment is accurate. Node.js's
timeoutoption tonet.connect()callssocket.setTimeout(timeout), which only emits a'timeout'event on socket inactivity; it does not automatically close the socket or emit'error'.Since the code only listens for
"connect"and"error"events, a timeout will leave the promise unresolved, causingwaitForPortto hang indefinitely when connecting to a black-hole address.The suggested fix to add explicit timeout handling is necessary:
- const connected = new Promise((resolve, reject) => { - const socket = connect({ host: hostname, port, timeout: 10_000 }); + const connected = new Promise((resolve, reject) => { + const socket = connect({ host: hostname, port, timeout: 10_000 }); socket.on("connect", () => { socket.destroy(); console.log("Connected:", `${hostname}:${port}`); resolve(); }); + socket.on("timeout", () => { + socket.destroy(new Error("Connection timed out")); + }); socket.on("error", error => { socket.destroy(); reject(error); }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.buildkite/ci.mjs(5 hunks)package.json(1 hunks)scripts/bootstrap.sh(24 hunks)scripts/machine.mjs(1 hunks)scripts/tart.mjs(5 hunks)scripts/utils.mjs(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time
Applied to files:
package.jsonscripts/tart.mjs.buildkite/ci.mjs
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Never use hardcoded port numbers in tests. Always use `port: 0` to get a random port
Applied to files:
scripts/utils.mjs
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr
Applied to files:
scripts/tart.mjsscripts/machine.mjs
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
.buildkite/ci.mjs
🧬 Code graph analysis (2)
scripts/tart.mjs (2)
scripts/machine.mjs (6)
result(126-136)result(138-138)result(1231-1231)result(1232-1232)result(1388-1388)snapshot(591-600)scripts/utils.mjs (14)
result(145-145)result(257-283)result(370-370)result(924-924)result(926-926)result(2725-2725)result(2791-2791)result(3061-3061)result(3065-3065)result(3079-3079)result(3161-3161)error(255-255)error(368-368)error(858-858)
.buildkite/ci.mjs (4)
scripts/machine.mjs (1)
cloud(1145-1145)scripts/utils.mjs (1)
os(1680-1680)scripts/runner.node.mjs (2)
os(353-353)platform(2062-2062)scripts/tart.mjs (1)
platform(46-46)
🪛 Biome (2.1.2)
scripts/tart.mjs
[error] 87-94: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🔇 Additional comments (17)
package.json (1)
95-98: LGTM!The new macOS machine configurations are well-structured and consistent with existing configurations. The
cloud=tartandarch=arm64choices are appropriate since Tart only supports darwin/aarch64 platforms..buildkite/ci.mjs (3)
106-109: LGTM!The expanded macOS platform support (releases 13, 15, 26) aligns with the new package.json configurations and tart.mjs distro mappings.
667-669: LGTM!Correctly excludes both
freebsdanddarwinfrom the release step dependencies, as darwin builds are handled separately via tart and aren't part of the main CI release flow yet.
1082-1082: LGTM!Appropriately excludes darwin platforms from automated image creation since tart-based image builds require a macOS host, which is outside the scope of the current CI infrastructure.
scripts/bootstrap.sh (6)
25-27: LGTM!The change from
exit 1tokill $$correctly handles the case whereerror()is called inside a subshell. Withexit 1, only the subshell would die and the script would continue. Usingkill $$ensures the entire script terminates.
458-460: Consider ifbrew upgradeis the intended behavior here.Running
brew upgradeduring package manager initialization will upgrade all installed packages, which can be time-consuming and may cause unexpected changes. Other package managers only runupdate(refresh package lists) at this stage.Was
upgradeintentional, or should this bebrew updateto match the behavior of other package managers?brew) - package_manager upgrade + package_manager update ;;
682-682: LGTM!The Homebrew installation uses the canonical install script URL from GitHub.
1320-1322: Verify the package namerustfor Homebrew.Homebrew uses the package name
rust, but this installs Rust via Homebrew's formula. This is correct, but note that Homebrew's Rust may lag behind the official releases.
1729-1739: LGTM!Proper darwin/aarch64 support added for the
ageencryption tool with the correct SHA256 hash for verification.
1169-1170: The original review comment's concern is not valid. Verification confirms thatlld@19(and other versioned variants) exist as separate Homebrew formulas. Modern Homebrew does packagelldas a separate versioned formula, not only as part ofllvm. The code at lines 1169-1170 correctly installs and links both packages separately, which is consistent with how other package managers handle these packages in the same script.scripts/machine.mjs (1)
34-34: LGTM!Clean refactor moving
spawnScptoutils.mjsfor centralized reuse across modules. The import aligns with the tart.mjs changes that also now importspawnScpfrom utils.scripts/tart.mjs (3)
2-2: LGTM!Correct imports added from
utils.mjsto support SCP and SSH functionality.
51-51: LGTM!Correctly adds macOS 26 (Tahoe) to the distro mapping, aligning with the new package.json configurations.
269-276: Placeholder implementation - confirm this is intentional.The
snapshot()method currently just returns the tag without actually creating or pushing a snapshot. The TODO comments indicate this needs further implementation to push to ghcr.io.Is this placeholder acceptable for this PR, with the actual implementation to follow? If so, consider adding a more explicit warning or throwing an error for now to prevent accidental misuse:
const snapshot = async tag => { console.warn("snapshot() is not yet implemented - returning tag as placeholder"); // TODO: push to ghcr and return that full remote_image_name:tag return tag; };scripts/utils.mjs (3)
118-128: SpawnOptions:detachedJSDoc addition is consistent and non-breakingThis just documents an option that
spawnalready forwards through tonodeSpawnvia...options, so it’s backward‑compatible and clarifies usage for new call sites.
2169-2196: getCloudMetadata: reduced retries to 3 keeps failures bounded but still safeDropping curl retries from 10 to 3 significantly shortens worst‑case wait time while still logging a warning and returning
undefinedon persistent failure. Given all callers already handle anundefinedresult, this change looks reasonable.
3028-3085: spawnSsh: lower defaultretriesto 3 — behavior change is localized and clearThe new
retries = 3default bounds the SSH retry loop more tightly while preserving the existing behavior of only retrying on exit code255without “Authenticated” in the verbose log. This looks fine, but it does reduce the grace period for slow‑to‑come‑up machines; if there are known slow environments, you may want to overrideretriesat those call sites rather than relying on the default.
| const { os, arch, distro, release, features } = platform; | ||
| const { publishImages } = options; | ||
| const action = publishImages ? "publish-image" : "create-image"; | ||
| const cloud = os == "darwin" ? "tart" : "aws"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer strict equality (===) for consistency.
The comparison uses == while the rest of the codebase uses === for string comparisons.
- const cloud = os == "darwin" ? "tart" : "aws";
+ const cloud = os === "darwin" ? "tart" : "aws";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cloud = os == "darwin" ? "tart" : "aws"; | |
| const cloud = os === "darwin" ? "tart" : "aws"; |
🤖 Prompt for AI Agents
In .buildkite/ci.mjs around line 618, the conditional uses loose equality (==)
to compare os to "darwin"; change it to strict equality (===) to match codebase
conventions and avoid type-coercion issues—update the expression so it uses ===
for the comparison while keeping the existing ternary result.
| return new Promise(async resolve => { | ||
| const result = await this.spawn(["get", name], { | ||
| json: true, | ||
| throwOnError: error => !/does not exist/i.test(inspect(error)), | ||
| }); | ||
| if (!result) resolve(undefined); | ||
| else resolve({ Name: name, ...result }); | ||
| }); |
There was a problem hiding this comment.
Remove unnecessary Promise wrapper with async executor.
The static analysis correctly flags this: async Promise executors are problematic because:
- Errors thrown in async executors won't reject the Promise—they'll be unhandled
- The
returnstatements insideresolve()calls don't exit the async function
The existing code also has a control flow issue: after resolve(undefined) on line 92, execution continues to line 93.
Simplify by removing the Promise wrapper entirely:
async getVm(name) {
- return new Promise(async resolve => {
- const result = await this.spawn(["get", name], {
- json: true,
- throwOnError: error => !/does not exist/i.test(inspect(error)),
- });
- if (!result) resolve(undefined);
- else resolve({ Name: name, ...result });
+ const result = await this.spawn(["get", name], {
+ json: true,
+ throwOnError: error => !/does not exist/i.test(inspect(error)),
});
+ if (!result) return undefined;
+ return { Name: name, ...result };
},Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 87-94: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🤖 Prompt for AI Agents
In scripts/tart.mjs around lines 87 to 94, remove the unnecessary new
Promise(async resolve => { ... }) wrapper and instead make the function body
directly async: await this.spawn([...], ...); if the spawn returns a falsy
result return undefined immediately, otherwise return { Name: name, ...result };
do not call resolve or use an async executor, and let thrown errors propagate
(or wrap in a normal try/catch if you need to convert errors) so control flow is
correct and no code runs after the early return.
scripts/tart.mjs has been broken since it was added — it called
spawnSsh/spawnSshSafe/spawnScp/spawnRdp without importing or defining
them, so any `machine.mjs --cloud=tart` invocation would crash on first
SSH/SCP. This unblocks the macOS image-build path.
- tart.mjs: import spawnSsh/spawnSshSafe/spawnScp from utils.mjs
- tart.mjs: drop dead rdp() (called non-existent spawnRdp; macOS uses
VNC, not RDP)
- tart.mjs: fix getVm() always-truthy bug — when `tart get` returns
nothing it now returns undefined instead of `{Name: name}`, so
cloneVm() correctly falls through to pull when the local image is
missing
- tart.mjs: switch base image suffix -xcode → -base (cirruslabs base
images are ~25GB vs ~50GB+ with full Xcode; bootstrap.sh installs
our own toolchain) and add macOS 26 Tahoe to the distros map
- tart.mjs: implement snapshot(label) — stop VM then `tart push` to
ghcr.io/oven-sh/<label>. machine.mjs:1559 already calls
machine.snapshot() on the publish-image path; without this the tart
cloud crashes there.
- tart.mjs: fix error message printing `undefined` instead of the
release number
- machine.mjs / utils.mjs: move spawnScp from machine.mjs to utils.mjs
(next to spawnSsh) so tart.mjs can import it without a cycle
Supersedes the tart.mjs portion of #25134 (drops the unrelated FreeBSD
and azure-removal changes from that branch).
scripts/tart.mjs has been broken since it was added — it called
spawnSsh/spawnSshSafe/spawnScp/spawnRdp without importing or defining
them, so any `machine.mjs --cloud=tart` invocation would crash on first
SSH/SCP. This unblocks the macOS image-build path.
- tart.mjs: import spawnSsh/spawnSshSafe/spawnScp from utils.mjs
- tart.mjs: drop dead rdp() (called non-existent spawnRdp; macOS uses
VNC, not RDP)
- tart.mjs: fix getVm() always-truthy bug — when `tart get` returns
nothing it now returns undefined instead of `{Name: name}`, so
cloneVm() correctly falls through to pull when the local image is
missing
- tart.mjs: switch base image suffix -xcode → -base (cirruslabs base
images are ~25GB vs ~50GB+ with full Xcode; bootstrap.sh installs
our own toolchain) and add macOS 26 Tahoe to the distros map
- tart.mjs: implement snapshot(label) — stop VM then `tart push` to
ghcr.io/oven-sh/<label>. machine.mjs:1559 already calls
machine.snapshot() on the publish-image path; without this the tart
cloud crashes there.
- tart.mjs: fix error message printing `undefined` instead of the
release number
- machine.mjs / utils.mjs: move spawnScp from machine.mjs to utils.mjs
(next to spawnSsh) so tart.mjs can import it without a cycle
Supersedes the tart.mjs portion of #25134 (drops the unrelated FreeBSD
and azure-removal changes from that branch).
3 participants
as is this gets
bun run machine:macos:14working when you havetartandsshpassinstalled.given the links in the comment for
tart.Machine.snapshotit looks like this would be integratabtle into CI proper without too much followup work, at least for arm64.testing with
[build images]but no need to[publish images]on merge yet13 screenshot
15 screenshot
26 screenshot
tartcan only be installed on macOS so darwin image builds will need to run on a macos queuethat should be easy to setup (can be another hosted queue) but feels outside the scope of this pr