ci: fix tart.mjs unresolved refs, add snapshot(), use -base images - #29315
ci: fix tart.mjs unresolved refs, add snapshot(), use -base images#29315alii wants to merge 14 commits into
Conversation
|
Updated 3:05 AM PT - Apr 17th, 2026
❌ @robobun, your commit bdf4cca has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29315That installs a local version of the PR into your bun-29315 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
6819067 to
7a13b20
Compare
9d417a6 to
9340cb3
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
scripts/tart.mjs:285-291— The PR removes therdp()function fromtart.toMachine()'s return object, butmachine.mjsunconditionally callsmachine.rdp()whenoptions.rdpis truthy — which happens with both--rdpand--vncflags. Since tart uses therdpoption to enablevnc-experimentalinrunVm(), passing--vnc --cloud=tartis a legitimate usage that will now throwTypeError: machine.rdp is not a function. Fix: guard the call site inmachine.mjswithif (typeof machine.rdp === "function"), or add a descriptive stub totart.toMachine().Extended reasoning...
What the bug is and how it manifests
Before this PR,
tart.toMachine()returned an object that included anrdpfunction (though it called the non-existentspawnRdp, causing a ReferenceError at call time). This PR explicitly drops it with the comment "macOS doesn't speak RDP anyway." The returned machine object now contains only:spawn,spawnSafe,attach,upload,snapshot,close, and[Symbol.asyncDispose]. There is nordpproperty, somachine.rdpisundefined.The specific code path that triggers it
machine.mjsparses CLI flags and sets:rdp: \!\!args["rdp"] || \!\!args["vnc"],
Then, unconditionally at the call site:
if (options.rdp) { await startGroup("Connecting with RDP...", async () => { const { hostname, username, password } = await machine.rdp(); ... }); }
There is no guard checking
typeof machine.rdp === "function"before invoking it.Why existing code doesn't prevent it
The
Machinetypedef marksrdpas optional (@property {() => Promise<RdpCredentials>} [rdp]), correctly signaling that not all cloud implementations need to provide it. However, the call site only checksoptions.rdp(the CLI flag), not whether the cloud-specific machine object actually exposes the method.What the impact would be
For tart, the
--vncflag is a meaningful option:tart.createMachine()passes'vnc-experimental': rdptorunVm(), which correctly enables VNC on the VM. A user runningnode scripts/machine.mjs create-image --cloud=tart --vncwould have the VM start with VNC enabled, but the process would immediately throwTypeError: machine.rdp is not a functionwhenmachine.mjstries to callmachine.rdp(). The VM remains running (orphaned) while the script crashes.How to fix it
Option A — guard the call site in
machine.mjs:if (options.rdp && typeof machine.rdp === "function") { await startGroup("Connecting with RDP...", ...); }
Option B — add a descriptive stub in
tart.toMachine():const rdp = async () => { throw new Error("macOS/tart does not support RDP; use --vnc to enable VNC-only access"); };
Step-by-step proof
- User runs:
node scripts/machine.mjs create-image --cloud=tart --os=darwin --arch=aarch64 --release=14 --vnc options.rdpis set totrue(line:rdp: \!\!args["rdp"] || \!\!args["vnc"])tart.createMachine()is called; it passes'vnc-experimental': rdp(i.e.,true) torunVm()— VNC is legitimately enabled on the VM.tart.toMachine(machineId)returns an object with nordpproperty.machine.mjschecksif (options.rdp)— this istrue.machine.rdp()is called — butmachine.rdpisundefined.TypeError: machine.rdp is not a functionis thrown. The VM is left running and orphaned.
- User runs:
-
🔴
scripts/tart.mjs:264-277— The PR removes therdp()stub fromtart.toMachine(), butmachine.mjsunconditionally callsmachine.rdp()whenoptions.rdpis truthy — which happens for both--rdpand--vncflags. Because--vnc --cloud=tartis a valid and meaningful flag combination (it enablesvnc-experimentalon the VM), users who pass it will immediately crash withTypeError: machine.rdp is not a function. Add atypeof machine.rdp === 'function'guard before the call site inmachine.mjs, or add a tart-specific stub that throws a descriptive error.Extended reasoning...
What the bug is and how it manifests
In
machine.mjs, themain()function buildsoptions.rdpas\!\!args['rdp'] || \!\!args['vnc']— so passing either--rdpor--vncmakesoptions.rdptruthy. Later, the code has an unconditional call:if (options.rdp) { await startGroup('Connecting with RDP...', async () => { const { hostname, username, password } = await machine.rdp();
After this PR,
tart.toMachine()returns an object without anyrdpproperty (the old stub was removed; the newsnapshotfunction replaced it). Somachine.rdpisundefined, and callingmachine.rdp()throwsTypeError: machine.rdp is not a function.The specific code path that triggers it
- User runs:
node scripts/machine.mjs create-image --cloud=tart --os=darwin --arch=aarch64 --release=14 --vnc options.rdpis set totrue(because\!\!args['vnc']istrue).tart.createMachine()correctly passes'vnc-experimental': rdptorunVm(), enabling VNC on the VM. This is intentional and useful.main()reachesif (options.rdp)— condition is true.machine.rdpisundefinedon the tart machine object.await machine.rdp()throwsTypeError: machine.rdp is not a function.
Why existing code does not prevent it
The
Machinetypedef marksrdpas optional (@property {() => Promise<RdpCredentials>} [rdp]), signaling that not all cloud implementations need to provide it. However, the call site inmachine.mjshas no guard liketypeof machine.rdp === 'function'— it only checksoptions.rdp(the CLI flag), not whethermachine.rdpis actually callable. Before this PR, tart'srdp()stub existed (though it called the nonexistentspawnRdp, producing aReferenceError); after this PR, the property is absent entirely, changing the failure mode to aTypeErrorat the call site rather than inside the function.What the impact would be
Any operator who passes
--vnc --cloud=tart(a legitimate option for obtaining graphical access to a macOS VM) will hit the crash before any VNC connection can be made. The VNC flag is wired up correctly on the tart side — the VM would start withvnc-experimentalenabled — butmachine.mjsaborts with a TypeError before doing anything useful.How to fix it
Option A — guard the call site in
machine.mjs:if (options.rdp && typeof machine.rdp === 'function') { await startGroup('Connecting with RDP...', async () => { const { hostname, username, password } = await machine.rdp(); ... }); }
Option B — add a descriptive stub in
tart.toMachine():const rdp = async () => { throw new Error('RDP is not supported on tart (macOS) VMs; use --vnc for VNC access'); };
Option A is safer for the general case (other future cloud implementations may also omit
rdp); option B gives a better error message.Step-by-step proof
args['vnc']is truthy sooptions.rdp = true.tart.createMachine()callstart.runVm(machineId, { ..., 'vnc-experimental': true })— the VM starts with VNC enabled. Correct.tart.toMachine(machineId)returns{ cloud: 'tart', id, spawn, spawnSafe, attach, upload, snapshot, close, [Symbol.asyncDispose] }— nordpfield.- Back in
main():if (options.rdp)evaluates totrue— enter block. machine.rdpevaluates toundefined.await machine.rdp()throwsTypeError: machine.rdp is not a function. Process crashes.
- User runs:
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).
- snapshot(): pass stdio:'inherit' to `tart push` so the ~25GB upload produces output and doesn't hit Buildkite's 10-min no-output timeout. Throw on missing label instead of pushing to ghcr.io/oven-sh/undefined. - toMachine(): fail fast with a brew hint when sshpass is missing (cirruslabs base images use password auth; spawnSsh shells to sshpass). - machine.mjs: fix Machine.snapshot typedef to take an optional label.
3e59d40 to
d29f055
Compare
…non-zero exit doesn't become an unhandled rejection
There was a problem hiding this comment.
LGTM — all previously flagged 🔴 issues have been addressed in this revision.
Extended reasoning...
Overview
This PR fixes which was completely broken since it landed: SSH/SCP helpers were called without being imported (causing ReferenceError), getVm() returned a truthy object even for non-existent VMs (preventing image pulls), and snapshot() was absent. The changes also move spawnScp from machine.mjs to utils.mjs to break a circular import dependency, switch cirruslabs base images from -xcode to -base variants, and add macOS 26 (tahoe) support.
Security risks
No security-sensitive code is touched. Auth credentials for the registry (TART_REGISTRY_USERNAME/TART_REGISTRY_PASSWORD) are read directly by the tart CLI from the environment — no new credential-handling code is introduced. SSH still uses password auth via sshpass (hardcoded admin/admin from cirruslabs base images), but this is the existing approach and is limited to local tart VMs.
Level of scrutiny
This is CI tooling (image-build scripts), not production user-facing code. The blast radius is confined to the macOS CI image-creation pipeline. The changes are well-scoped: bug fixes with clear before/after intent, no novel algorithms or protocol changes.
Other factors
All six rounds of inline comments (flagging unhandled rejection in runVm, VM orphan on missing sshpass, silent push output, orphaned VM on toMachine throw) are marked resolved and the fixes are visible in the current diff. Remaining open issues are pre-existing minor cosmetics (undefined username in SCP error message, stdio:inherit nullifying stderr capture) inherited verbatim from machine.mjs — not regressions introduced here.
|
@robobun adopt |
|
✅ Adopted — up to date with Since picking up:
All review threads resolved. |
The Machine typedef marks rdp as optional, and tart's toMachine() no longer returns it (macOS uses VNC, not RDP). Without this guard, --vnc --cloud=tart crashes with 'machine.rdp is not a function' after the VM is already running.
… into claude/tart-mjs-fixes
The getVm() null-check fix makes cloneVm()'s registry-pull path reachable for the first time. Without stdio:inherit, tart's ~25GB layer download produces no terminal output and Buildkite's 10-min no-output watchdog kills the job mid-pull. Same pattern as snapshot()'s push.
… into claude/tart-mjs-fixes
942c656 on main bumped scripts/build/deps/mimalloc.ts to a29368e but left the hardcoded expectation in process.test.js at 9a5e1f5, so the process.versions test fails on every platform that does a fresh build. Unblocks CI here until #29295 (derive expectations from the dep files) lands and removes the hardcoding entirely.
There was a problem hiding this comment.
LGTM — all the previously flagged blockers have been addressed across the iteration commits.
Extended reasoning...
Overview
This PR fixes a series of longstanding bugs in scripts/tart.mjs that had prevented machine.mjs --cloud=tart from ever completing: missing SSH/SCP imports (ReferenceError), getVm() always-truthy bug suppressing remote image pulls, the dead rdp() referencing non-existent spawnRdp, and a missing snapshot() implementation. It also moves spawnScp to utils.mjs to avoid circular deps, switches from -xcode to -base cirruslabs images, and adds macOS 26 (Tahoe) support.
Security risks
No security-sensitive code paths are touched. The hardcoded admin:admin credentials in connect() are the cirruslabs default for local tart VMs and are expected; the sshpass pre-flight check is gated before any VM resources are allocated. No auth logic, crypto, or permission code is modified.
Level of scrutiny
Medium. This is CI infrastructure / scripting code, not production bun runtime code. The changes are incremental fixes to a module that was previously broken, so the risk of regression to the broader codebase is low. The test plan is necessarily limited (full macOS VM tests require a Mac with tart installed), but the static correctness checks (--check, module import smoke test) pass.
Other factors
The PR went through thorough review with multiple rounds of bug reports and fixes. All previously flagged red bugs were addressed: sshpass guard moved before resource allocation, throwOnError:false on the unawaited tart run promise, { stdio: "inherit" } on long-running clone and push operations (preventing Buildkite no-output timeouts), and the mimalloc hash sync in process.test.js. The one remaining nit (missing imageId/instanceType/region in toMachine()) is purely cosmetic — it only affects a diagnostic console.table printout.
… into claude/tart-mjs-fixes
…able machine.mjs prints these in the 'Created machine:' console.table after createMachine() returns; aws and azure populate them. tart has no cloud metadata, so use the OCI image string, a cpu/mem summary, and 'local'.
… into claude/tart-mjs-fixes
… into claude/tart-mjs-fixes
There was a problem hiding this comment.
LGTM — all critical issues addressed.
Extended reasoning...
Overview
This PR fixes long-standing breakage in scripts/tart.mjs: missing imports for SSH/SCP helpers, an always-truthy getVm() that prevented remote image pulls, a missing snapshot() implementation, and switches from -xcode to lighter -base cirruslabs images. It also relocates spawnScp from machine.mjs to utils.mjs to avoid a circular dependency, and adds a guard for the machine.rdp call site.
Security risks
No security-sensitive code touched. Auth for registry pushes relies on env vars (TART_REGISTRY_USERNAME/TART_REGISTRY_PASSWORD) read directly by tart — no credentials are handled in JS. The hardcoded admin/admin SSH credentials are standard for cirruslabs base images on a local VM network.
Level of scrutiny
This is CI tooling — scripts that build and publish macOS VM images. It is not in any production or runtime path. The changes are mechanical: import fixes, a null-check, stdio inheritance for long-running spawns, and a new snapshot() method. Low-risk CI scripting changes that warranted review but not extended scrutiny once the critical bugs were fixed.
Other factors
All five red/yellow inline bugs from prior review rounds were resolved by robobun (commits c28e1ad, e465ec1, d2bbd54, 6761fa6, 7aa892d, and others). All review threads are marked resolved. The one remaining new finding (JSDoc typedef label? should be label) is cosmetic — no current call site omits the label — and is called out in an inline comment.
Stacked on #29314. Part 2 of the macOS CI Phase 0 prerequisites — unblocks
scripts/machine.mjs --cloud=tartso we can build golden macOS VM images.Problem
scripts/tart.mjshas been broken since it landed:toMachine()callsspawnSsh,spawnSshSafe,spawnScp, andspawnRdpwithout importing or defining any of them. The first SSH/SCP into a Tart VM throwsReferenceError. Separately,getVm()returns{Name: name}even when the VM doesn't exist (always truthy), socloneVm()never pulls a missing base image.Changes
spawnSsh/spawnSshSafealready exist inutils.mjs; just import them.spawnScpfrommachine.mjs→utils.mjs(next tospawnSsh) sotart.mjscan import it without a circular dep.rdp()— called non-existentspawnRdp; macOS doesn't speak RDP anyway.getVm()always-truthy — returnundefinedwhentart getfinds nothing.-xcode→-baseimages + add macOS 26 — cirruslabs-baseimages are ~25 GB vs ~50 GB+ with full Xcode; we install our own toolchain viabootstrap.sh.snapshot(label)— stop the VM, thentart push ghcr.io/oven-sh/<label>.machine.mjsalready callsmachine.snapshot()on thepublish-imagepath; tart was the only cloud missing it. Auth viaTART_REGISTRY_USERNAME/TART_REGISTRY_PASSWORD(tart reads them directly).undefinedinstead of the unsupported release number.Supersedes the
tart.mjsportion of #25134 (drops that branch's unrelated FreeBSD support and azure-import removal so this stays scoped).Test plan
node --check scripts/{tart,machine,utils}.mjsbun -e 'import {tart} from "./scripts/tart.mjs"'— module loads, no unresolved refstart.getImage({os:"darwin",arch:"aarch64",release:"14"})→ghcr.io/cirruslabs/macos-sonoma-basetart.toMachine("x")returns{snapshot, upload, spawn, spawnSafe, attach, close, ...}import {spawnScp} from "./scripts/utils.mjs"resolvesnode scripts/machine.mjs create-image --cloud=tart --os=darwin --arch=aarch64 --release=14boots a VM and runs bootstrap (requires ci: split downloadCacheDir from cacheDir, add BUN_DEPS_CACHE_PATH #29314'sbootstrap.shdarwin fixes — separate follow-up)