feat(compile): standalone executables, verified on macOS, Linux and Windows - #536
feat(compile): standalone executables, verified on macOS, Linux and Windows#536colinhacks wants to merge 297 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…f silence Making --install-message purely additive had inverted the default into silence, so a binary compiled without the flag unpacked ~100 MB of Node with no output at all -- a multi-second apparent hang on first launch. The flag now customizes the text and cannot suppress it; omitting it takes the default. `None` in the manifest still means "print nothing" (the launcher contract is unchanged), but nub compile never emits it.
…ange check Two defects found by building and running real executables, not by reading the diff. A --smol first run leaked 792 bytes of curl progress meter to stderr on a pipe. The meter was muted only while the first-run box owned the terminal, so a non-interactive run -- the case the docs promise sees "nothing at all" -- got the full progress table. Muted off a TTY as well; an interactive run without the box still gets it. Verified on a real 606 KB smol binary against a fresh HOME on a pipe: stderr 792 bytes -> 0, app output intact. The docs claimed a --smol binary "may carry a range, checked against the Node it finds at runtime". It is not checked: `node_range` is baked into every smol manifest and read nowhere in the launcher, which accepts any discovered Node >= the floor regardless of major. Compiling with `--target 22.x` and running on Node 26 was verified to succeed. The `>=` rule is deliberate (main.rs documents it as the orchestrator default), so this corrects the documentation to describe the floor semantics and states plainly that no upper bound is enforced, rather than overriding that decision. Whether `node_range` should be enforced or dropped is a separate call; today it is dead weight in every smol binary.
`nub compile` resolves its launcher template as a sibling of the running `nub`, but nothing ever built or published one, and the release binary did not enable the `compile` feature at all — so the verb could not work for any user. Build crates/nub-launcher per platform in the release matrix and copy it into the platform package's bin/ as `nub-launcher-<platform>[.exe]`, next to the binary. That one placement covers every channel: npm ships all of bin/, and github-release tars the same bin/ for install.sh, install.ps1 and `nub upgrade`. Homebrew installs it alongside the binaries, and the Windows self-owned upgrade — which swaps per-file rather than renaming the whole dir — now refreshes every bin/ sidecar, fixing the same latent staleness for busybox.exe. Release builds gain `compile` alongside `embed-runtime`; the feature lists are disjoint and the deps are compile-time only. crates/nub-core's manifest becomes self-contained. The launcher is its own cargo workspace, so `cd crates/nub-launcher && cross build` mounts only that workspace and its path deps — without the repo-root manifest, nub-core's `.workspace = true` fields have no root to inherit from and the parse fails. That is the #132 failure one crate over; crates/nub-cache-key carries the same note for the same reason. Cargo.lock is unchanged, so the inlined literals resolve identically. A release still ships only the host's launcher, so cross-compiling to a foreign platform remains unsupported; the docs Callout says so.
Add the two asset flags from the locked flag surface. --include embeds a file, directory, or glob byte-for-byte; --exclude prunes within what was included, and matching nothing is silent (an --include matching nothing is an error, since a typo would otherwise surface as a missing file on a user's machine). Assets ride the existing app payload region rather than a region of their own, so the container format is unchanged and the launcher half needs no edit: ensure_app already creates parent directories, already rejects an escaping name, and app_sha256 already hashes every entry's name and bytes, so a changed asset set re-keys the extraction dir for free. The extracted app dir mirrors the source tree, anchored at the deepest directory containing both the entry and every included path, so the relative geometry between the entry and its assets survives compilation and import.meta.url/__dirname paths resolve to the same files. With no --include the anchor is the entry's own directory, leaving a no-asset build laid out exactly as before. Assets are materialized as real files rather than served from a virtual filesystem (VFS dismissed 2026-07-24): nub extracts a real Node either way, so a VFS would buy nothing while costing fs-API fidelity.
…ames at build time Review of the --include/--exclude change found two silent failures, both rooted in matcher tokens being compared lexically while the entry arrives canonicalized. An --exclude spelled through a symlink prefix-matched nothing, and an unmatched --exclude is silent by design, so `--include . --exclude "$PWD/.env"` embedded the secret without a word — $PWD is the logical path while current_dir() is the physical one, making that the ordinary spelling in a build script rather than an exotic input. The same mismatch on --include collapsed the anchor to the filesystem root, producing a binary that compiled cleanly and then failed to find its assets at runtime. Include and exclude roots are now resolved through their deepest existing ancestor, so both sides land in one namespace. Globs are matched below the include root instead of against the absolute build-machine path, so a project directory named `my[work]` can no longer be read as glob syntax, and a path that exists verbatim is taken literally so a Next.js `app/[id]` route directory can be embedded at all. An empty token is rejected rather than selecting the whole working tree, which is what an unset variable in `--include "$ASSET_DIR"` produced. is_safe_relative_name moves to nub-core so the compiler and the launcher share one predicate: payload names are user-derived now, and a name the launcher would refuse has to fail the build rather than ship an executable that aborts on someone else's machine.
…sed for matching The previous commit resolved include and exclude tokens through their symlinks outright. That fixed the spelling it was written for and broke the mirror-image ones, because anchoring and matching genuinely want different answers. Naming wants the tree the user described: with `public` a symlink into a sibling package, resolving it moved the asset outside the entry's tree and the app's own ../public path stopped resolving once compiled. Matching wants every spelling of one file to compare equal: an --exclude naming a symlinked file did not prefix-match the path its --include had walked, and an unmatched exclude is silent, so the named file shipped anyway. Ancestors are now reconciled while the final component stays exactly as written, so a token spelled /tmp against an entry canonicalized to /private/tmp still shares a directory while a symlinked leaf keeps its name. Excludes additionally compare fully resolved forms, so either spelling prunes. The payload-name safety check moves to assemble_app, the one point where every name exists — it previously ran over asset names only, leaving prefixed bundle chunks and the synthesized package.json ungated. A glob tail no longer matches its own root, so a trailing /* cannot prune the directory it was meant to descend into.
* compile: key --smol flag injection to the discovered Node, scan version managers, drop dead node_range Three defects in the launcher, all on the --smol path, all verified against real compiled executables rather than by reading the diff. Flag injection was computed from `manifest.node_version`, which for --smol is the acceptance FLOOR, not the Node actually discovered at runtime. A binary compiled `--smol --target 22` running on Node 26.5.0 got the 22.x band: it injected --experimental-wasm-modules and --experimental-detect-module (both default-on no-ops by 26) while withholding --experimental-eventsource, -addon-modules, -import-text, -ffi, -vfs and -stream-iter, so node:ffi, node:vfs, node:stream/iter and native text imports were all silently unavailable. On Node 26.2.0 it also injected --enable-source-maps, which nub deliberately withholds on that patch band. acquire_node now returns the Node AND its own concrete version; embed still uses the manifest, since it bakes the exact binary. This is the "thread the discovered version through later" refinement recorded in the design. Discovery probed only nub's own store and PATH, so a machine with a usable Node under ~/.nvm downloaded another one -- version managers are shell-hook based and put nothing on a non-interactive PATH, which is exactly how a compiled binary is launched. Added nvm, fnm, Volta, asdf and mise, with layouts read from those tools' own sources; newest satisfying install wins, and the libc gate the embed dedup already applies now covers these external trees too. node_range was baked into every --smol manifest and read nowhere. Enforcing it would reverse the recorded acceptance rule (any discovered Node >= target, whatever the major), which is a product decision, not a defect fix -- so the dead field is removed instead. The docs already describe floor semantics and state that no upper bound is enforced, so they need no change. * compile: probe the discovered Node's accepted flags, and fix the version-manager roots Self-review findings on the previous commit. Keying the flag band to the discovered version is necessary but not sufficient: that version is inferred from a directory name, and the launcher passed no accepted-flag set, so it was the one spawn path in the tree without the intersection that makes injection self-correcting (nub run uses it on both of its). A version dir naming a Node it does not hold aborts startup with no fallback -- verified: a tree with `v26.9.0` holding Node 22.15.0 exits 9 with five `node: bad option` lines, and exits 0 with the probe. Node acquisition now reports whether it is Managed (embedded or just provisioned by nub, version exact) or Discovered (found on the host), and only the latter pays the probe, which caches per (path, mtime). Three discovery-root defects. An empty or relative NVM_DIR/FNM_DIR/... made every root CWD-relative, so a matching tree beside the process would be scanned and its node executed; env overrides are now absolute-only. nvm's non-env default is $XDG_CONFIG_HOME/nvm when that is set, which was missed. And the Windows roots were all wrong -- Volta is %LOCALAPPDATA%\Volta, fnm is %APPDATA%\fnm, mise is %LOCALAPPDATA%\mise, nvm-windows has no versions/node segment -- so the scan was a guaranteed no-op there. Roots are now a per-platform table. Home resolution moved to dirs_next, which falls back to getpwuid_r when HOME is unset. That is precisely the launch context this feature exists for (a service manager or cron that sourced no profile), where reading $HOME alone returned None and the whole scan silently no-opped. Verified: with HOME unset the scan now finds the real ~/.nvm Node. Also skip a present-but-non-executable bin/node so it loses to the next candidate instead of failing at spawn with a bare permission error, and extend the scanner test to cover the libc gate and the exec bit through best_node_in -- the existing assertions used a darwin triple, which short-circuits that gate.
…ve (#625) Rolldown honors a tsconfig compilerOptions.jsx of "preserve", which writes raw JSX into the bundle; the compile reported success and the binary died with Unexpected token '<' on first run. Against opencode, 7 of 309 emitted chunks failed node --check, one of them a shared config chunk. Four changes: - Re-parse every emitted chunk with oxc before the payload is injected. A chunk that is not valid ESM fails the compile, naming the chunk and the position. - Override jsx: preserve to the automatic runtime, matching what nub run already does, and only when the entry's effective tsconfig says preserve so a classic runtime project keeps Rolldown's per-file choice. - Resolve module before main, so a legacy dual package's UMD build is not picked over its ESM one. - Extend the unresolved gate to require(): an unguarded call whose require is a local binding cannot be rewritten and throws MODULE_NOT_FOUND in the artifact. Guarded and non-static calls are exempt — every such site in opencode's tree was an optional-dependency probe. The compile feature is off by default, so CI's bare cargo test never built any of this; the test job now runs the compile-pipeline tests on one leg.
`nub <file>` transpiles to es2022, which lowers `using`/`await using` into the oxc `usingCtx` helper. The bundler was never told a syntax target, so a compiled artifact kept the declaration verbatim and died with a SyntaxError on Node 22 — after a build that reported success. The target names an ES year rather than the target's Node version, which is the only spelling that works: oxc's `EngineTargets::from_target` inserts a default ES engine beside whatever you name, and its feature lookup returns on the ES entry the moment it sees one, so a bare `node22.15` lowers nothing. It is unconditional, matching the runtime, so there is no Node for which the compiler lowers less than `nub <file>` would.
`runtime/compile-preamble.mjs` is a second implementation of what the run-time preload installs, and a second implementation falls behind the first silently: a global added to the preload and not the preamble just vanishes when compiled. The existing fixture checks a hand-picked list of APIs is callable. This one names nothing, so it catches the polyfill nobody thought to list — it digests every own property of globalThis plus the members of the builtins nub patches rather than replaces, and the harness compares that against the reference run. Audited both bands before adding it: the preamble installs exactly what the preload does (22 additions on Node 22.15, 9 on Node 26.5, none missing and none extra). Watched failing with the preamble emptied, 415 names against 407.
…g policy Three additions to the compiled-executables design doc. What deliberately does not survive: the `nub` command's own surfaces, watch-mode `import.meta.hot` (a type-only shape that is undefined outside `nub watch --hot`, so an artifact matches an ordinary run exactly), run-time configuration, and Node version selection. An unlisted absence reads as a defect, so each is named. Strict compilation, stated as a policy: every config surface is honored when the artifact is built and read by nothing when it runs, with no opt-in in either direction. Verified against a directory carrying six hostile config files at once; the `nub` CLI fails there on nub.jsonc while the artifact is unaffected. Records the one Node-inherited caveat, a malformed package.json meeting the `--require` preload, which reproduces on plain node with no Nub involved. The syntax level the bundler emits, including why the target names an ES year rather than the target's Node version, and the `--node-flag` rename.
…s boundary A worker is a second entry point and needs everything the main thread gets. The bundler already emits it as a real chunk behind a generated wrapper; nothing tested that the thread actually receives it. The fixture runs on Node 22.15, where a verbatim worker source would not run at all, and checks an erased enum alongside two polyfilled globals read from inside the thread. Also records where this stops. `new Worker` names its argument as code; a path handed to `child_process.fork` is a string indistinguishable from a path to data, so it is embedded as a data asset and the child runs unaugmented. The build already warns by name and points at the shape that is bundled properly. Resolving arbitrary spawn arguments statically is unbounded, so the warning is the deliberate stopping point rather than a gap to close.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The using fix is the right call and its reasoning holds up — I verified the oxc EngineTargets claim against the pinned source. Two things to settle before merge: the new harness contains a fixture that can never pass, so run.sh exits non-zero on every run, and the unconditional target now transforms bundled dependency code that nub <file> deliberately leaves alone.
Reviewed changes — the four commits since the 17d9755 review.
- Down-levelled emitted syntax to a fixed ES year —
jsx_overrideincrates/nub-cli/src/compile/bundle.rsnow always returns aBundlerTransformOptionsand always setstarget: SYNTAX_TARGET("es2022"), sousingno longer reaches the artifact verbatim and die at parse time on Node 22. - Added a unit test on the emitted bundle —
using_declarations_are_lowered_for_the_targetasserts the entry chunk contains oxc'susingCtxhelper rather than asserting the keyword's absence. - Added a three-way augmentation differential —
tests/compile-augmentation/run.shplus nine fixtures, comparing plain Node,nub <file>and the compiled artifact, with the plain-Node column as a positive control and a per-fixture.differsfile for augmentations that are supposed to disappear when compiled. - Covered the transform augmentations by identity — new TypeScript, JSX, decorator-metadata and module-resolution fixtures, the last of which asserts which file answered each import rather than that something did.
Two things I checked and found sound, so they do not need re-litigating: the doc comment's claim that naming a bare node22.15 lowers nothing is exactly right (EngineTargets::from_target_list in oxc 0.140.0 unconditionally inserts Engine::Es → ESTarget::default(), which is ESNext, and has_feature returns on that entry the moment it visits it), and es2022 neither strips static import attributes nor changes decorator handling — crates/nub-native/src/detect.rs:51-59 lists import attributes among what es2022 does not lower, and jsx_override never touches oxc's separate decorator field.
compile/bundle.rs is still rendered as (binary file or no changes) in the formatted diff, so the section below could not be anchored inline; its line references are to the working tree at 3db848d.
⚠️ The new syntax target transforms dependency code the runtime deliberately never touches
SYNTAX_TARGET is described as "the SAME one nub <file> transpiles to". The value matches runtime/transform-core.mjs:725 exactly, but the scope does not: the runtime treats excluding node_modules from the transform as a load-bearing invariant it calls "the byte-parity boundary", while Rolldown applies BundlerTransformOptions to every module in the graph. The runtime also carries a documented fallback for the case where oxc rejects a construct V8 accepts; the compile path has none, so a dependency of that shape turns into a hard build failure rather than a file that ships as-is.
Technical details
# `SYNTAX_TARGET` widens oxc's transform to every bundled dependency, with no fallback
## Affected sites
- `crates/nub-cli/src/compile/bundle.rs:3539` — `const SYNTAX_TARGET: &str = "es2022"`, and `:3513`
where it is set unconditionally on `BundlerTransformOptions.target`. Before this commit `transform`
was `None` unless the entry's tsconfig said `jsx: "preserve"`, so plain-JS dependency modules never
reached oxc's transformer at all.
- Rolldown `=1.2.0` (`crates/nub-cli/Cargo.toml:63`) applies the option bundler-wide:
`ModuleTask::run_inner` → `create_ecma_view` → `parse_to_ecma_ast` → `PreProcessEcmaAst::build`
runs the `Transformer` for every resolved module. The only `node_modules`-aware check in that path
(`is_local_project_file`) gates an annotation *warning*, not the transform step.
- `runtime/preload-async-hooks.mjs:97` and `:112`, `runtime/transform-core.mjs:181` and `:785` — the
runtime's `node_modules` exclusion, described there as "the byte-parity boundary" and
"make-or-break".
- `runtime/transform-core.mjs:817-834` — the `#225` class: a file oxc's stricter ES grammar rejects
but V8 accepts (`set x(v = []) {}` in pnpm 11.x's bundled `pnpm.mjs`). The runtime falls back to the
original source. `crates/nub-cli/src/compile/bundle.rs:412-416` converts any bundler error into
`anyhow!("the bundler failed:\n{}", …)` with no retry-untransformed path.
- Second-order effect: a `v`-flag RegExp literal inside a dependency is now rewritten to
`new RegExp(…, "v")` (`crates/nub-native/src/detect.rs:54-55`, `:241-247`). Behaviour-preserving,
but it is evidence the transformer is running over dependency code.
## Required outcome
- Either the compiler's transform scope matches the runtime's, or the doc comment's parity claim is
narrowed to the ES-year value and the widened scope is stated deliberately — including what happens
when oxc rejects a dependency the previous code path shipped untouched.
- If the wider scope is intentional (a compiled artifact arguably *should* down-level dependency code,
since it must parse on the target's Node), the `#225` failure mode needs a disposition: a hard build
failure naming the offending file is acceptable, silently failing with `the bundler failed` is not.
## Open questions for the human
- Is transforming dependency code the intent here, or a side effect of reaching for a single global
`target`? The two have different answers: the first wants a diagnostic for the reject case, the
second wants the target scoped to project sources the way the runtime scopes it.
- Related, and worth deciding now rather than later: rolldown's `should_transform_js()` gate bug
([rolldown/rolldown#10564](https://github.com/rolldown/rolldown/pull/10564), fixed in v1.2.2) skips
the transformer entirely for targets in the ES2024–ES2025 band. `es2022` sits below it, so the
pinned `=1.2.0` is safe today — but raising `SYNTAX_TARGET` without moving the pin would silently
reinstate the exact `using` bug this commit fixes.ℹ️ Nitpicks
crates/nub-cli/src/compile/bundle.rs:3489-3490— the rewritten doc comment kept half of the old sentence: "Rolldown otherwise honorspreserveby emitting raw JSX — syntax no JavaScript / otherwise honors by emitting raw JSX — syntax no JavaScript engine parses, so". The second line is the leftover.- No workflow runs the new harness —
grep -rn "compile-augmentation" .github/ Makefilereturns nothing, whilecompile-native.yml:130and:157do runtests/compile-native-islands/run.shandtests/compile-corpus/layouts.sh. That is consistent withtests/compile-corpus/run.shalso being manual-only, so it may be deliberate — but it means the always-red fixture above would not have surfaced on its own. wiki/design/compiled-executables.md:281still describes the differential as "a fixture run twice … against Node 26.5 and again against 22.15". The harness now runs it three ways, defaults to a single26.5.0, and has a.differsexception class the section does not mention. The transform-time row at:271also does not name the syntax level, which is now one of the augmentations that travels.
Claude Opus ｜ 𝕏
| NODE_VERSION="${1:-26.5.0}" | ||
| WORK="${WORK:-${TMPDIR:-/tmp}/nub-compile-augmentation}" | ||
|
|
||
| rm -rf "$WORK"; mkdir -p "$WORK"; cd "$WORK" || exit 1 |
There was a problem hiding this comment.
$WORK is reset once here, before the loop, so each fixture's .d payload persists into every later row. With the glob order at :53 (*.mjs, then *.ts, then *.tsx), a-typescript.ts — which has no .d of its own — runs under a-resolution.d's tsconfig.json, and the first .d/package.json copied in replaces the npm init -y manifest, dropping the react/reflect-metadata entries installed at :35. Each row's inputs are then a function of glob order rather than of the fixture.
`import icon from "./icon.png"` yields a path inside a compiled binary and throws ERR_UNKNOWN_FILE_EXTENSION under `nub app.ts` — the runtime imports the data formats and text, not images, fonts, media or opaque payloads. Nothing said so, and the failure lands in the dev loop before the user ever compiles. Points at reading the file by URL, which behaves the same either way.
…ed ones The icon is set through libsui in the same builder chain as the payload write, which the existing comment there already called for: libsui rebuilds the PE resource directory from scratch, so anything baked into the launcher template would be discarded by the injection. Because this is byte editing rather than a Windows API call, it works from any host — Bun documents its equivalent as unusable when cross-compiling. Validated by content, not extension: a PNG saved under a .ico name is the ordinary mistake and would embed into a resource Windows declines to draw, invisible until someone opens Explorer on another machine. Non-Windows targets refuse the flag rather than accepting and ignoring it, since macOS reads an icon from the .app bundle and Linux from a .desktop entry. Verified against a real win32-x64 PE by parsing the output's resource directory: the iconed build carries RT_ICON and RT_GROUP_ICON, the control only RT_RCDATA. Version metadata (publisher, description, copyright) is not included — libsui exposes no VS_VERSIONINFO writer, so it needs a hand-built resource and is a separate piece of work.
Bun ships isStandaloneExecutable; the equivalent is declined because what makes a program need to ask is usually a virtual filesystem, and nub extracts real files and points process.execPath at the binary. Anyone who needs the answer has it via an identity test against the launcher's private path variable — presence alone is wrong, since the variable is inherited and a forked child would claim to be the executable.
The harness is the acceptance test for the erasability goal — every fixture runs as `nub <file>` and as the compiled artifact, and the two must agree. It is the only gate that catches an augmentation the runtime makes and the compiler does not: the class that produced a Node 22 SyntaxError after a build reporting success, and data-format imports that could not be compiled at all. Until now it only ran when someone ran it by hand. Linux only and ~53s, alongside the layout-shapes harness already there. Uses the runner's own Node so nothing is provisioned, which also makes the plain-Node control the same build as the reference run and the artifact. Also adds the two harness directories to the path filters. Neither tests/compile-augmentation nor tests/compile-corpus triggered this workflow, so either could have been edited to green without anything checking.
There was a problem hiding this comment.
ℹ️ No critical issues. One rough edge in the new
--iconflag — the check refuses a file that would have worked, on a mechanism the libsui vendored in this PR does not have — plus one wiki paragraph the PR's own corpus fixture disproves.
Reviewed changes — the six commits since the 3db848d review.
- Added
--iconfor Windows executables —injectgrew anicon: Option<&[u8]>parameter and now calls libsui'sset_iconin the same PE builder chain aswrite_resource, so the icon survives the resource-directory rebuild the payload write performs.load_iconreads and checks the file before the launcher template is fetched, and refuses the flag outright on a non-PE target. - Added a global-surface parity fixture —
a-global-parity.mjsdigests every own property ofglobalThisplus the members of the builtins Nub patches rather than replaces, so a polyfill added to the run-time preload and not tocompile-preamble.mjsfails without anyone having named it in advance. - Added a worker augmentation fixture —
a-worker.tsplusa-worker.d/worker-entry.ts, pinning that anew Worker(new URL(…))chunk gets its TypeScript transpiled and the preamble installed inside the thread. - Said that binary asset imports are compile-only —
compile.mdxnow states thatimport icon from "./icon.png"throwsERR_UNKNOWN_FILE_EXTENSIONundernub app.ts, and namesreadFile(new URL(…))as the form that works both ways. - Recorded what an artifact deliberately drops — new wiki sections covering the four absences, the strict-compilation policy, and why there is no public compiled-binary detection API.
Four things I checked rather than assumed, so they do not need re-litigating. All four Bun claims the new commits make are accurate against Bun's live documentation: --windows-icon really is documented as unusable when cross-compiling, Bun.isStandaloneExecutable really exists, the .env/bunfig.toml autoload set and the "may also be disabled by default for more deterministic behavior" sentence match exactly, and the new Worker(path) quote is verbatim and still current. The icon and the payload coexist safely: the root resource table ends up RT_ICON(3), RT_RCDATA(10), RT_GROUP_ICON(14), which is the ascending order the Windows loader binary-searches for — though that is a consequence of the call order rather than of any sort, and find_in_pe scans linearly, so no round-trip test could ever catch a regression there. import { Worker } from "node:worker_threads" is a recognised spelling — classify_worker_callee proves it, and the CommonJS require binding is refused rather than silently shipped as data. The __NUB_COMPILED_EXEC_PATH identity test is described correctly, including the empty-string spelling and the cloned-Worker fallback.
ℹ️ --icon refuses a file that would have produced a correct icon, and the reason it gives is not what the code does
load_icon rejects anything whose first four bytes are not [0, 0, 1, 0], telling the author that "a PNG or JPEG renamed to .ico embeds but does not draw". The libsui vendored in this same PR never embeds the bytes: set_icon decodes them with the image crate — whose png and bmp features are enabled — and re-encodes six resolutions as fresh ICO resources. A PNG saved under a .ico name is exactly the input the check names, and it would have worked. The same false mechanism is stated in four places, one of them user-facing.
Technical details
# `load_icon`'s gate refuses working input on a mechanism `set_icon` does not have
## Affected sites
- `crates/nub-cli/src/compile/mod.rs:433-440` — the `bytes.get(..4) != Some(&[0, 0, 1, 0])` gate and
its error text ("a PNG or JPEG renamed to .ico embeds but does not draw. Convert it first.").
- `crates/nub-cli/src/compile/mod.rs:409-416` — the doc comment, which opens "refusing anything that
would produce a Windows executable with a broken resource directory" and then attributes the
breakage to libsui embedding the file.
- `site/content/docs/compile.mdx:696` — the user-facing repetition: "a PNG renamed to `.ico` embeds
but never draws, so the build checks the contents and refuses one rather than shipping an
executable with a blank icon."
- `crates/nub-cli/src/compile/mod.rs:1866-1868` — the test docstring repeats it a fourth time.
- What `set_icon` actually does: `vendor/libsui/lib.rs:200-202` is
`ImageReader::new(Cursor::new(icon)).with_guessed_format()?.decode()?`; `vendor/libsui/Cargo.toml:32-39`
enables `image` features `bmp`, `ico` and `png`; `vendor/libsui/lib.rs:232-254` resizes the decoded
image to 256/128/48/32/24/16 and re-encodes each as ICO data. The input bytes never reach the
executable.
## Second, smaller problem: the gate does not deliver the fail-fast it exists for
- `crates/nub-cli/src/compile/mod.rs:120-122` — "Read before the expensive work, so a bad path or a
mislabelled file fails in the first second rather than after a ~100 MB Node download."
- Magic bytes are not decodability. A truncated, malformed, or otherwise undecodable `.ico` passes
`load_icon` and fails inside `set_icon`'s `decode()?` at `crates/nub-cli/src/compile/inject.rs`,
i.e. after the bundle, the Node provisioning and the whole build. The one input class the early
check catches is a file that is not an image at all.
- The unit test at `mod.rs:1870` writes a six-byte ICO header, which `set_icon` could never decode —
so nothing in the suite passes a decodable icon through the injection path either.
## Required outcome
- The reason stated in the comment, the error text, the test docstring and `compile.mdx` matches what
the code does. Either the accepted set matches what `set_icon` accepts, or the `.ico` restriction is
kept and justified by something true.
- Whatever the accepted set becomes, the early check validates membership in it, so the promise at
`mod.rs:120-122` holds for the inputs that actually fail.
## Suggested approach (optional)
- Decode in `load_icon` instead of sniffing four bytes. `image` is already in the tree as a libsui
dependency, so a direct dep is close to free, and one `ImageReader::with_guessed_format()?.decode()?`
both widens the gate to what libsui supports and moves the real failure to the first second.
## Open questions for the human
- Is ICO-only deliberate product policy — matching what a Windows user expects to hand a build tool,
and matching Bun's own flag — or was it written to avoid a failure mode that turns out not to exist?
If it is policy, the four texts need a different reason rather than a different gate.ℹ️ Nitpicks
- The two new fixtures land in a harness that still cannot go green.
tests/compile-augmentation/run.shis untouched by this delta, soa-env-strictstill pins[ "$fail" = 0 ]to failure on every run, anda-worker.d/is now a third.dpayload inheriting the never-reset$WORK— meaninga-worker.tsruns undera-resolution.d'stsconfig.json. Both are the open threads from the previous review rather than new defects, but they are what decides whether these two new guards can fail for the right reason. wiki/design/compiled-executables.md:293states the parity fixture's measured result — "22 additions over plain Node on 22.15, 9 on 26.5, none missing and none extra" — and the harness that would reproduce it is the one above, which no workflow runs and which currently exits non-zero regardless. Worth noting where the number came from, since the committed guard cannot currently corroborate it.
Claude Opus ｜ 𝕏
|
|
||
| **A child process started from a path the compiler was not told is code.** A `Worker` is a second entry point and is treated as one — `new Worker(new URL(…, import.meta.url))` becomes a real chunk behind a generated wrapper, so the worker thread gets its TypeScript transpiled and the preamble installed inside the thread. Verified on Node 22.15, where a raw worker source would not run at all: `enum` erased, `Temporal` and `reportError` both present in the thread. Bun documents the same case as one it does not yet handle. | ||
|
|
||
| `child_process.fork(…)` is the shape that does not get this, and the difference is information rather than effort: `new Worker` names its argument as code, while a path handed to `fork` is a string that the compiler cannot distinguish from a path to data. Such a file is embedded as a **data asset** — shipped verbatim, never transpiled, its own imports resolving against an extraction directory with no `node_modules` — so reading it works and executing it does not, and a forked child runs without the augmentations its parent has. The build says so, naming the file and pointing at the `Worker` shape that is bundled properly. Chasing the general case means statically resolving arbitrary spawn arguments, which is unbounded; the warning is the deliberate stopping point. |
There was a problem hiding this comment.
This paragraph overstates the limitation twice. "executing it does not" is contradicted by this PR's own tests/compile-corpus/fixtures/a-fork.mjs, which does fork(new URL("./fork-child.mjs", import.meta.url)) and asserts the child runs and answers over IPC — executing an embedded data asset works whenever it needs no transpile and no node_modules. And "a path handed to fork is a string" names the one spelling that is not embedded and gets no build-time note: warn_module_data_assets fires only for the new URL(literal, import.meta.url) shape.
Technical details
# The `child_process.fork` paragraph describes a stricter failure than the code produces
## Affected sites
- `wiki/design/compiled-executables.md:313` — "Such a file is embedded as a **data asset** … so
reading it works and executing it does not" and "The build says so, naming the file".
- `tests/compile-corpus/fixtures/a-fork.mjs` — forks `new URL("./fork-child.mjs", import.meta.url)`,
sends a message and asserts `ok:42` off the reply. A committed fixture in this PR that executes an
embedded data asset successfully.
- `crates/nub-cli/src/compile/bundle.rs:2605-2616` — `warn_module_data_assets`, the note the paragraph
refers to. It is driven by the `new URL(…)` scan (`scan_new_urls`), so it never sees a bare string
argument to `fork`.
## Required outcome
- The paragraph distinguishes the two spellings: `fork(new URL(…))` embeds the file, warns, and runs
it fine unless it needs a transpile or a bare-specifier resolution; `fork("./child.mjs")` embeds
nothing and gives no build-time signal at all, which is the worse of the two and is currently
described as the covered one.
There was a problem hiding this comment.
Important
This wires the augmentation harness into CI, which is the right move — but the harness still exits non-zero on every run for the reason flagged on the previous review, so as written this step turns the compile-native Linux leg permanently red.
Reviewed changes — the one commit pushed while the previous review was being submitted.
- Ran the compile-augmentation harness on CI — a new
Compiled artifacts keep Nub's augmentationsstep oncompile-native.yml's Linux leg, invokingtests/compile-augmentation/run.shwith the runner's own Node version so the plain-Node control, the reference run and the artifact are all the same build. - Widened the workflow's path filters —
tests/compile-augmentation/**andtests/compile-corpus/**on both thepushandpull_requesttriggers, so a change to either harness triggers the workflow that now runs them.
The Node-version choice is right and its reasoning holds: run.sh:41-42 computes PLAIN_NODE before the loop and falls back to command -v node, so passing ${version#v} really does make all three columns the same build rather than introducing a version gap.
⚠️ The step this adds cannot pass, because a-env-strict still cannot pass
run.sh and the fixtures directory are untouched by this commit, so the a-env-strict row still scores as a failure on every run and [ "$fail" = 0 ] at run.sh:112 still exits 1. Promoting the harness to a CI step with set -euo pipefail is what converts that from a local annoyance into a red workflow on every push and PR that touches the widened path filters.
Technical details
# The new CI step fails deterministically on the first run
## Affected sites
- `.github/workflows/compile-native.yml:173-183` — the new step. `set -euo pipefail` means `run.sh`'s
exit status is the step's.
- `tests/compile-augmentation/run.sh:112` — `[ "$fail" = 0 ]`, the harness's exit status.
- `tests/compile-augmentation/fixtures/` — still has no `a-env-strict.d/`, and nothing in `run.sh`
writes a `.env`. All three columns print `ok:dotenv=unset`, so `got == ref`, and the `.differs`
branch at `run.sh:89-94` scores that equality as FAIL. This is the open thread on `run.sh:67` from
the previous review, unchanged.
- `tests/compile-augmentation/run.sh:29` and `:67` — the second open thread. `$WORK` is reset once
before the loop, so each fixture's `.d` payload persists into every later row. That matters more now
than it did locally: a red CI leg whose row inputs are a function of glob order is hard to attribute.
## Required outcome
- The harness exits 0 on a healthy tree before it gates anything. Either `a-env-strict` gets the `.env`
it needs (and an assertion that attributes the difference to compile-time strictness rather than to
the artifact's working directory, which `run.sh:74` currently changes), or the fixture is removed
until it can.
- Ideally the two open `run.sh` threads land in the same change, so a CI failure names one fixture
rather than a glob-order interaction.
## Open questions for the human
- Was this step verified against a real run before wiring, or added on the expectation that the
harness was green? The answer decides whether anything else in the harness is also red on CI's Node
version — several rows go vacuous there, and the `.d` persistence means `a-typescript.ts` runs under
`a-resolution.d`'s `tsconfig.json`.Claude Opus ｜ 𝕏
| version="$(node --version)" | ||
| NUB="$nub" \ | ||
| __NUB_LAUNCHER_TEMPLATE="$GITHUB_WORKSPACE/crates/nub-launcher/target/release/nub-launcher" \ | ||
| tests/compile-augmentation/run.sh "${version#v}" |
There was a problem hiding this comment.
This invocation cannot succeed today. run.sh and tests/compile-augmentation/fixtures/ are untouched by this commit, so there is still no a-env-strict.d/ and nothing writes a .env — all three columns print ok:dotenv=unset, the .differs branch scores that equality as a failure, and run.sh:112 exits 1. With set -euo pipefail above, that is the step's status, so this leg goes red on every push and PR matching the widened path filters.
The repo ignores all dotfiles, so the fixture's .env was never committed. It
existed only on the machine that wrote it, which meant the harness passed
locally off an uncommitted file and, the first time it ran in CI, the reference
run had no .env to load — both columns said "unset", the row could not differ,
and the assertion failed looking exactly like a product regression.
Un-ignored by an explicit negation rather than `git add -f`: a force-added file
stays ignored and reappears as the same trap for whoever edits it next. The
neighbouring tests/fixtures/env-scope/.env is tracked-but-ignored that way today.
Also notes in the fixture why it does not probe for the .env itself — the
`new URL("./.env", import.meta.url)` that would answer the question is the exact
expression the bundler turns into an embedded asset, which would put the file in
the payload and destroy what is being tested.
There was a problem hiding this comment.
ℹ️ The deterministic failure from the last review is genuinely fixed. One residual on the same fixture's assertion, inline.
Reviewed changes — the one commit since the 37699d2 review, b4870e6 test(compile): track the .env the augmentation harness needs.
- Staged the
.envthea-env-strictfixture needs —tests/compile-augmentation/fixtures/a-env-strict.d/.envholdingFROM_DOTENV=leaked, sorun.sh:67's.dcopy finally fires for that fixture. - Un-ignored it —
.gitignore:13-17adds!tests/compile-augmentation/fixtures/*/.envbeneath the broad.*rule, with a comment saying why the fixture has to be a real dotfile. - Recorded why the fixture does not look for the file — a comment on
a-env-strict.mjsnoting thatnew URL("./.env", import.meta.url)is the expression the bundler embeds, so probing for the.envwould put it in the payload.
I checked each of the three columns rather than taking the fix on trust. ref is now ok:dotenv=leaked: env_file_names_for_mode in crates/nub-core/src/workspace/env.rs yields .env for every resolved mode including the empty one, reached from cli.rs:3794. got is ok:dotenv=unset for the right reason — runtime/compile-preamble.mjs, runtime/compile-bootstrap.cjs and crates/nub-launcher/src/main.rs contain no env-file loading at all. plain is unset, so the row is discriminating. The .gitignore negation is genuinely effective: no parent directory of the file is excluded, so git's re-inclusion restriction does not bite, and git ls-files confirms the file is tracked. The comment about new URL is accurate — NewUrlAssets (compile/bundle.rs:2262-2288) does embed a literal specifier.
ℹ️ Nitpicks
- The
.envis now a third.dpayload persisting into every later row of the harness (the still-open thread onrun.sh:29). Verified harmless today — no other fixture readsprocess.env, anda-global-parity.mjsdigests own property names rather than env contents — but it does meana-env-strictfinding thenpm init -ymanifest as its project root depends on it sorting first in the glob rather than on anything the fixture states. .gitignore:13-17sits inside the repo-root dotfile allowlist (:8-19), while the existing precedent for a fixture-path negation lives further down at:122-123(!tests/fixtures/env-test/.env). Grouping the two keeps the root-dotfile list scannable as a list of root dotfiles.- Whether the CI step this unblocks now exits 0 end-to-end still is not establishable from the diff:
a-typescript.tsanda-worker.tscontinue to inherita-resolution.d'stsconfig.jsonby glob order. Worth a localNUB=… tests/compile-augmentation/run.sh "$(node --version | tr -d v)"before merging, since the step is now the gate rather than a local harness.
Claude Opus ｜ 𝕏
| [ -f "$HERE/fixtures/$name.differs" ] && reason="$(head -1 "$HERE/fixtures/$name.differs")" | ||
|
|
||
| if [ -n "$reason" ]; then | ||
| if [ "$got" != "$ref" ]; then |
There was a problem hiding this comment.
Now that a-env-strict can reach this arm, note what satisfies it: got != ref is also true when the build failed, because :76 sets got="<build failed: …>" — so a compile error specific to this fixture scores as ok (differs on purpose: …). The same inequality cannot separate compile-time strictness from :74's cd "${TMPDIR:-/tmp}", which is the residual from the thread on :67.
Technical details
# The `.differs` pass arm accepts a build failure and cannot attribute the difference
## Affected sites
- `tests/compile-augmentation/run.sh:90` — `if [ "$got" != "$ref" ]`. The only condition for a
`.differs` fixture to pass.
- `tests/compile-augmentation/run.sh:75-77` — on a failed `nub compile`, `got` becomes
`<build failed: …>`. That string is never equal to `ref`, so the pass arm at `:91` is taken and
`pass` is incremented. Before `b4870e6` this was unreachable for `a-env-strict`, because `ref` was
also `ok:dotenv=unset` and the row always took the FAIL arm at `:93`.
- `tests/compile-augmentation/run.sh:74` — the artifact runs from `${TMPDIR:-/tmp}`, the parent of
`$WORK`. An artifact that DID load a `.env` from its working directory would still print `unset`
there, so the row would stay green.
- `tests/compile-augmentation/fixtures/a-env-strict.differs` — one line, the reason string. It carries
no expected value.
## Required outcome
- A `.differs` row passes only when the artifact produced the output the policy predicts, not merely
something other than `ref`. In particular a failed build must not score as a pass on any row.
## Suggested approach (optional)
- Give the `.differs` file a second line holding the expected artifact output
(`ok:dotenv=unset`) and assert `got` equals it, falling back to the current inequality when the
second line is absent. That closes both the build-failure hole and the attribution gap in one
change, and keeps the reason beside the fixture.A stack trace is the augmentation a user meets on their worst day. `nub <file>` resolves one through the transpile; the compiler has to reach the same answer from a map built ahead of time, and the pipeline producing those mappings is the same one the syntax target and JSX settings run through — so it can regress from a change aimed somewhere else entirely. Audited it and found no defect: inline and linked maps both resolve to the original .ts line, external writes a map for an error tracker without resolving at run time, and the default ships no map, which is also what keeps your source out of what you distribute. Compares file and line only. The absolute paths differ by construction and the columns shift a few characters under minification, but the line is the fact a person needs, and the trace goes to "unresolved" the moment mapping breaks. Source maps are off by default, so this fixture cannot be tested without asking for them: a `<name>.flags` sidecar now carries per-fixture compile flags, beside `<name>.differs` and for the same reason — a table in the runner would drift from the fixtures it describes.
…h it
Merging the v0.7.0 release turned every Test leg red across all six OS/Node
combinations: `process.versions.nub` reported 0.6.0 from a 0.7.0 binary.
nub-core inlines its version rather than inheriting the workspace one, and that
is deliberate — it is a cross-workspace path dependency of crates/nub-launcher,
whose release `cross` build mounts only that workspace, so an inherited field
would have no root to resolve against. The release was cut from a tree where
nub-core still inherits, so the bump moved the workspace and left the inlined
literal behind. It matters past packaging: spawn.rs hands the preload
env!("CARGO_PKG_VERSION") from nub-core, not from the binary, so a stale literal
makes the runtime misreport its version to everything downstream.
This is the second time — 0.5.0 to 0.6.0 failed identically, and the fix then
said it had extended version-check to the inlined manifests. It had not: the
gate reads the npm packages, the root Cargo.toml and runtime/version.mjs, and no
crate manifest at all, so it passed while the bug was live. It now checks both
inlined manifests, and was watched failing on the real mismatch before the value
was corrected. `scripts/set-version.mjs` already rewrites both, so an ordinary
release keeps them in step; this is the backstop for when one does not.
There was a problem hiding this comment.
ℹ️ No critical issues. The
.flagsmechanism is careful and the new fixture can genuinely fail; two scoping suggestions inline.
Reviewed changes — the one commit since the b4870e6 review, 3b2dd52 test(compile): pin that a compiled crash names the line the author wrote.
- Added a per-fixture compile-flags file —
run.sh:72-79readsfixtures/<name>.flagsone flag per line intocompile_flags=(), reset per row so nothing leaks between fixtures, and:81splices it into thenub compileinvocation. - Added the stack-trace fixture —
a-sourcemap.tsthrows at its own line 11, reads frame 1 of the stack and printsok:<basename>:<line>, comparing only file and line;a-sourcemap.flagscarries--sourcemap=inlinebecause a compiled binary ships no map unless asked.
I checked that the row can fail for the right reason rather than trusting its shape. With the map absent the artifact's frame is the minified bundle, the /([^/\\]+\.ts):(\d+):\d+/ match misses, and got becomes ok:unresolved:… — never equal to ref; with a wrong map the line differs. --enable-source-maps does reach the artifact, through the launcher's own flags::compute_inject_flags call (crates/nub-launcher/src/main.rs:285). And nothing shifts line 11 before the map is written: hoist_module_wrappers is a no-op on a fixture with no imports and no CommonJS wrapper (compile/bundle.rs:1717-1720), and the compile root wrapper is a separate two-line virtual source rather than a prefix spliced into the authored module, asserted across all three sourcemap modes by compile_root_wrapper_keeps_source_maps_and_output_paths_separate (bundle.rs:6321). The ${compile_flags[@]+"${compile_flags[@]}"} spelling is the right one — the alternate value is internally quoted, so elements do not word-split, and the + test is what keeps an empty array safe under set -u on bash 3.2.
ℹ️ Nitpicks
- The vacuity counter will label this row
vacuous. The test isplain != ref(run.sh:111-115), and Node's strip-only type stripping is position-preserving, sonode app.tsalso printsok:app.ts:11— the one fixture whose subject is compile-only lands in the bucketrun.sh:13-17describes as "the fixture would pass with nub deleted", which is the opposite of true here. A sentence in the fixture's header comment saying its evidence lives in the artifact column would keep the summary line readable. tests/compile-augmentation/is the only one of the three compile harnesses with noREADME.md(tests/compile-corpus/andtests/compile-native-islands/both have one). There are now two per-fixture side-file conventions —.differsand.flags— discoverable only by readingrun.sh, and the harness is a CI step rather than a local script.while read -r flag(run.sh:78) drops the last line of a.flagsfile that has no trailing newline.a-sourcemap.flagshas one, so this is latent;while read -r flag || [ -n "$flag" ]closes it.
Claude Opus ｜ 𝕏
| // | ||
| // Only file and line are compared. The absolute paths differ by construction (the | ||
| // artifact reports a path inside its extraction directory) and the columns differ | ||
| // by a few characters after minification, but the line is the fact a person needs. |
There was a problem hiding this comment.
This row fails on Node 26.2.x for a non-defect: nub deliberately withholds --enable-source-maps on that band (crates/nub-core/src/node/flags.rs:37-39, honored at :263-270), so ref reports the transpiled line while the minified artifact reports no .ts at all and got becomes ok:unresolved:…. CI is safe today — compile-native.yml:82 pins node-version: '24' — but run.sh takes the version as an argument and labels rows (vacuous on $NODE_VERSION), which invites the sweep that hits the band.
Technical details
# `a-sourcemap` fails on the one Node band where nub withholds `--enable-source-maps`
## Affected sites
- `crates/nub-core/src/node/flags.rs:37-39` — `source_maps_safe` returns `false` for
`major() == 26 && minor() == 2`, for an `assert.ok(false)`-rethrows-as-`TypeError` regression.
- `crates/nub-core/src/node/flags.rs:263-270` — `compute_inject_flags` `continue`s past
`--enable-source-maps` when that predicate is false. Both the `nub <file>` spawn path and the
launcher call this, so the withholding is symmetric — but its OUTCOME is not.
- `tests/compile-augmentation/fixtures/a-sourcemap.ts:19-21` — with no remapping, `ref` still names
`app.ts` (nub serves the transpiled text under the same URL) but at the transpiled line, since oxc
codegen drops the fixture's nine leading comment lines. `got` names the minified bundle, so the
regex misses entirely and prints `ok:unresolved:<40 chars>`.
- `tests/compile-augmentation/run.sh:26` — `NODE_VERSION="${1:-26.5.0}"`; the default and CI's Node 24
are both clean. `run.sh:114` appends `(vacuous on $NODE_VERSION)`, which is what tells a reader the
harness is meant to be run per version.
## Required outcome
- A supported Node version on which nub deliberately disables source maps does not produce a red row
that reads as a compile regression.
## Suggested approach (optional)
- Skip the row when `source_maps_safe` would be false — a `<name>.skip` side file holding a version
predicate, or a guard in the fixture itself that prints the `ref` value when
`process.sourceMapsEnabled` is false, so the row stays green and honest rather than being deleted.| @@ -0,0 +1 @@ | |||
| --sourcemap=inline | |||
There was a problem hiding this comment.
inline is the mode the docs advise against (site/content/docs/compile.mdx:147, "Prefer linked") and the one with the least machinery — the map is a data URI already inside the chunk. linked is the recommended mode and the only one where the map is a separate payload entry that has to survive embedding, extraction and a relative sourceMappingURL resolve, and nothing anywhere runs an artifact built that way.
Technical details
# The first run-the-artifact sourcemap guard covers the mode with the least plumbing
## Affected sites
- `tests/compile-augmentation/fixtures/a-sourcemap.flags:1` — `--sourcemap=inline`.
- `crates/nub-cli/src/compile/bundle.rs:603-611` — only `External` detaches the maps; `Linked` leaves
them in `files`, i.e. as ordinary payload entries the launcher extracts, named by the chunk's
`//# sourceMappingURL=` comment and resolved relative to the extracted chunk. Commit `80a0fbe3`
("ship the source map beside the bundle instead of inside it") is that plumbing.
- `site/content/docs/compile.mdx:147` — recommends `linked` on the grounds that Node reads it only
when it renders a trace, so it is the mode most users will have on.
- Coverage: `grep -rn sourcemap crates/nub-cli/tests/integration.rs tests/` finds no test that runs a
compiled artifact under any sourcemap mode. The bundler-level tests
(`an_unused_asset_is_dropped_under_every_sourcemap_mode`, the `SourcemapMode::Linked` case at
`bundle.rs:6513`) assert on emitted text and never execute a binary, so a map that is embedded but
lands where Node cannot find it passes all of them.
## Required outcome
- The mode whose map depends on payload embedding and extraction is exercised by a run of the
artifact, not only by bundler-output assertions.
## Suggested approach (optional)
- A sibling fixture with `--sourcemap=linked` in its `.flags` — the mechanism this commit adds makes
that a two-file addition, and the two rows together separate "the map is generated correctly" from
"the map reaches the extracted tree".
There was a problem hiding this comment.
ℹ️ The version carry is correct and
make version-checkpasses at v0.7.0. Two things the new gate leaves behind, inline.
Reviewed changes — the two commits since the 3b2dd52 review.
- Merged
origin/main(269045e) — main's 0.7.0 release bump (rootCargo.toml,crates/nub-native/Cargo.{toml,lock},runtime/version.mjs, all tennpm/*/package.json) plus main's ownsite/content/docs/**,site/public/schema/*and skill work. No branch product code changed. - Carried
nub-coreto 0.7.0 —crates/nub-core/Cargo.toml:21, which the merge left at0.6.0because theset-version.mjsstamp for that manifest exists only on this branch. - Extended
version-check—Makefile:199-204now asserts the inlinedversionline incrates/nub-core/Cargo.tomlandcrates/nub-native/Cargo.tomlequalsV, with an error namingprocess.versions.nubas the stake.
I ran the project's own scorer rather than tallying the surfaces by hand: make version-check reports ✓ All npm packages, Cargo.toml, and runtime/version.mjs at v0.7.0; @oxc-project/runtime matches nub-native oxc pin (0.140.0). The new loop's /^version = "([^"]*)"/m is non-global, and each target manifest has exactly one ^version = line, so it reads the [package] version in both. Restricting the loop to those two crates is right rather than an omission — nub-launcher (0.0.0), nub-phantom (0.1.0), nub-data-formats (0.1.0), nub-cache-key (0.2.0) and nub-json-guard (0.1.0) all carry deliberately independent versions, and nub-cli inherits from the workspace.
Claude Opus ｜ 𝕏
| # failing loudly. (`version` is cosmetic: nub-core is `publish`-less and | ||
| # `scripts/set-version.mjs` does not stamp it, same as nub-cache-key.) |
There was a problem hiding this comment.
This parenthetical says the opposite of what the branch does, one line above the version this commit just bumped. scripts/set-version.mjs:83 stamps this manifest — with its own seven-line comment explaining that spawn.rs hands env!("CARGO_PKG_VERSION") to the preload as process.versions.nub — and the version-check loop this commit adds now fails the build when it drifts. A reader trusting this comment is the reason it drifted.
| # failing loudly. (`version` is cosmetic: nub-core is `publish`-less and | |
| # `scripts/set-version.mjs` does not stamp it, same as nub-cache-key.) | |
| # failing loudly. `version` is NOT cosmetic and NOT unstamped: | |
| # `scripts/set-version.mjs` writes it and `make version-check` fails on drift. |
| const mm = t.match(/^version = \x22([^\x22]*)\x22/m); \ | ||
| if (!mm) errors.push(m + ': inlined version line not found — it must NOT inherit from the workspace (see the manifest comment)'); \ | ||
| else if (mm[1] !== v) errors.push(m + ' has ' + mm[1] + ', expected ' + v + ' — an inlined version is what spawn.rs hands the preload as process.versions.nub, so a stale one makes the runtime misreport itself'); \ | ||
| } \ |
There was a problem hiding this comment.
The gate reads manifests, so the lockfile that references one of them is still stale: crates/nub-launcher/Cargo.lock:909-910 pins nub-core 0.6.0. make version refreshes only the root lock and crates/nub-native/Cargo.lock, and the launcher is a third committed lockfile this PR adds — so cargo silently rewrites it on the next build and leaves a dirty tree.
Technical details
# The third committed lockfile is not carried with `nub-core`'s version
## Affected sites
- `crates/nub-launcher/Cargo.lock:909-910` — `name = "nub-core"` / `version = "0.6.0"`, against a
manifest that now says `0.7.0`. `nub-launcher` is its own workspace (excluded at `Cargo.toml:49`)
taking `nub-core` as a cross-workspace path dependency, and its lockfile is committed by this PR.
- `Makefile` `version` target — runs `cargo update -p nub-cli -p nub-cache-key -p nub-core --precise
$(V)` for the root lock and `cd crates/nub-native && cargo update -p nub-native --precise $(V)`.
Nothing touches `crates/nub-launcher/Cargo.lock`; the target's success line already claims "both
Cargo.lock files".
- `Makefile:199-204` — the new loop reads the two manifests and no lockfile.
## Blast radius
- Not a CI break today: every launcher leg is plain `cargo`/`cross build` with no `--locked`
(`ci.yml:258`, `:408`, `:957`, `:975`; `compile-native.yml:107`, `:236`; `release.yml:811`). Note
`ci.yml:207` DOES pass `--locked` for `crates/nub-native`, so the convention exists and the launcher
is the one workspace outside it.
- What ships is a committed lockfile that disagrees with its own workspace, which cargo repairs
in-place — so the next contributor to build the launcher gets an unrelated diff in their tree.
- I could not run the decisive `cargo metadata --locked` probe here: this runner has no registry
cache, so `--offline` fails on `anyhow` for an unrelated reason.
## Required outcome
- A release bump carries every committed lockfile that records a bumped crate's version, and the
refreshed `crates/nub-launcher/Cargo.lock` lands with this change.
## Suggested approach (optional)
- Add `@cd crates/nub-launcher && cargo update -p nub-core --precise $(V)` to the `version` target
beside the `nub-native` line, and correct that target's "both Cargo.lock files" message.Every augmentation result so far came from the default embed shape. --smol is a different code path: it carries no Node blob and finds one at run time, so it computes the injected flag set against a version the build never saw — which is the stated reason the flag policy computes rather than bakes. Leaving the shape that motivates the design as the untested one is the wrong way round. Audited before wiring it up, and found no defect: all fixtures pass under --smol on Node 26.5 and again on 22.15, where all of them discriminate. A COMPILE_FLAGS variable adds flags to every fixture's build rather than a second runner, and the summary line names the shape so a pass cannot silently be the one already covered.
`process.versions.nub` means "Nub's augmentation is active in this process" — the contract the runtime's own tests pin, and why `--node` and NODE_COMPAT both withhold it. A compiled artifact withholds it as well, but only because the compile bootstrap never sets the variable: nothing asserted it and nothing wrote it down, so the behaviour was an accident that happened to be defensible. It is a real fork. The artifact's augmentations genuinely are active — the other fixtures reproduce 22 polyfilled globals on Node 22 — so a library gating on the marker takes the plain-Node branch inside a compiled binary and gives up capability that is present. The other reading wins anyway: an artifact is a standalone program on stock Node, not a `nub` process, and the marker exists to detect the latter. Feature-detect the capability instead and the answer is the same either way. Pinned with a `.differs` fixture so the absence cannot drift back into being accidental, and recorded in the design doc where the choice can be reversed.
There was a problem hiding this comment.
ℹ️ No new issues. The
--smolleg is worth having and the flag splicing is right — but the step's comment names the one thing this configuration cannot cover and omits the three it does.
Reviewed changes — the one authored commit since the 3a8b426 review, touching two files.
- Added a
--smolleg to the augmentation harness — a second Linux-only step incompile-native.yml(new lines 188-199) runstests/compile-augmentation/run.shwithCOMPILE_FLAGS=--smol, otherwise identical to the step above it. - Threaded shape flags through the runner —
run.sh:34readsCOMPILE_FLAGSinto an array and:88-89splices it after each fixture's own.flags;:128-129names the shape in the summary line, so a green run cannot be mistaken for the one already covered.
I checked that the new leg really is a different code path rather than a second run of the same one, and it is: a discovered Node makes NodeOrigin::Discovered gate the accepted_env_flags intersection (crates/nub-launcher/src/main.rs:281-284, where the embed shape is Managed and passes None), the app cache drops to CacheUse::DataOnly (:1026), and no polyfill is stripped at all (compile/bundle.rs:1494, :7934). The splicing idiom is safe — read -r -a on an empty value leaves a zero-element array, and ${EXTRA_COMPILE_FLAGS[@]+"…"} expands to nothing under set -u. I did not run the harness (no release binary here), but I traced each fixture's observable and found no code-grounded reason for the step to be red: every polyfill is typeof-guarded before it installs, and this PR already unified the webstorage predicate between spawn.rs and the launcher, so both columns inject the same set on Node 24.
ℹ️ Nitpicks
rm -rf ./cache(run.sh:90) deletes the whole cache base, and under--smolthat includes any Node the launcher provisioned —provision_smol_nodewrites tobase.join("node")(crates/nub-launcher/src/main.rs:1726), where base is$WORK/cache/nub. Latent today because the CI step passes the PATH Node's own version, so discovery always succeeds; a run handed a version argument that is not onPATHwould download a Node once per fixture. Removing only./cache/nub/compile-appand./cache/nub/compile-node(main.rs:941,:947) keeps the app extraction cold without that.COMPILE_FLAGSsplits onIFS, so it cannot carry a flag whose value contains a space —--node-options='--max-old-space-size=64 --trace-warnings'arrives as two arguments — while the per-fixture.flagsreader, which takes a whole line per element, handles exactly that. Worth a word in the header comment if the variable stays space-separated.- The new step re-runs the harness from scratch, so
npm i react reflect-metadata(run.sh:42) and both control columns are paid a second time for a delta that only changes the artifact column, and:43exits 2 when that install fails — the registry dependency doubles with it. Looping the shapes insiderun.shwould compile twice per fixture and install once.
Claude Opus ｜ 𝕏
| # The other shipping shape, and a whole code path the default never touches: | ||
| # --smol carries no Node blob and finds one at run time, so it computes the | ||
| # injected flag set against a version the build never saw. That is the | ||
| # reason the flag policy is what it is, so it should not be the untested one. |
There was a problem hiding this comment.
The stated reason is the one thing this configuration cannot exercise. run.sh:37 writes an exact .node-version, so the pin is VersionPin::Exact → smol_exact_target: true (compile/mod.rs:298, :650-652) and compile/mod.rs:233-258 bakes that exact version with no provision_version; the launcher then accepts only that version (nub-launcher/src/main.rs:1249-1265), so the flag set is computed against precisely the version the build resolved. What the step does newly cover is real and goes unnamed — the discovered-Node flag intersection, the data-only cache posture, and the full unstripped polyfill set.
Technical details
# The `--smol` leg's comment names an uncoverable reason and omits three real ones
## Affected sites
- `.github/workflows/compile-native.yml:184-187` — "it computes the injected flag set against a
version the build never saw". Needs a NON-exact pin; `tests/compile-augmentation/run.sh:37`
writes `printf '%s\n' "$NODE_VERSION"`, and the step passes the runner's own `node --version`.
- `tests/compile-augmentation/run.sh:23-27` — the same claim, in the harness header.
- `crates/nub-cli/src/compile/mod.rs:298` + `:650-652` — `smol_exact_target` is true for an exact
pin, so `smol_candidate_matches` rejects every other version.
- `crates/nub-cli/src/compile/mod.rs:233-258` — for an exact pin the floor IS the resolved version
and `newest` is filtered to `None`, so `manifest.provision_version` is empty.
- What the leg actually adds, none of it named: `crates/nub-launcher/src/main.rs:281-284`
(`NodeOrigin::Discovered` is what gates `discovery::accepted_env_flags`; embed is `Managed` at
`:1071` and passes `None`), `:1026` (`CacheUse::DataOnly` for a proved external Node), and
`crates/nub-cli/src/compile/bundle.rs:1494` + `:7934` (`--smol` strips no polyfill, so the
artifact carries the whole set where the embed shape drops the known-native ones).
## Required outcome
- The comment states what this row verifies, so a future reader does not conclude the
version-divergence path is covered when it is not.
## Open questions for the human
- Is covering a discovered version the build never resolved worth its own row? It needs a non-exact
pin, which also breaks this harness's "all three columns are the same Node build" invariant — so
it is a separate fixture and a separate decision, not a flag on this one.| # The other shipping shape, and a whole code path the default never touches: | |
| # --smol carries no Node blob and finds one at run time, so it computes the | |
| # injected flag set against a version the build never saw. That is the | |
| # reason the flag policy is what it is, so it should not be the untested one. | |
| # The other shipping shape, and code paths the default never touches: the | |
| # artifact DISCOVERS its Node, so the injected set is intersected with what | |
| # that Node still accepts, the app cache is data-only, and no polyfill is | |
| # stripped because the target runtime is unknown at build time. The pin the | |
| # harness writes is exact, so the discovered version is the one the build | |
| # resolved — covering a version the build never saw needs a non-exact pin. |
There was a problem hiding this comment.
ℹ️ No new issues. The marker decision is correct and its premise checks out against the contract tests that already pin it; the one gap is where the decision is written down.
Reviewed changes — the one commit pushed while the previous review was in flight, f26d5e3 test(compile): decide and pin the self-identification marker. No product code changed: the behavior is unchanged and now asserted.
- Pinned the withheld marker —
tests/compile-augmentation/fixtures/a-version-marker.mjsprintsok:marker=<process.versions.nub ?? "absent">, with a.differsfile inverting the assertion so the artifact is required to disagree with thenub <file>reference. - Recorded the decision in the design doc —
wiki/design/compiled-executables.md:317-322states the fork, why it is deliberate rather than an oversight, and that a library should feature-detect the capability instead.
The premise holds where it counts. crates/nub-cli/tests/process_versions_nub.rs already pins all three sides of the contract — augmented nub publishes the marker (:62), --node must not (:84), NODE_COMPAT=1 must not (:96) — and crates/nub-core/src/node/spawn.rs:986-990 couples VERSION_ENV to the augment block with a comment saying --node skips it for free, so a compiled artifact's absence is the same shape rather than a new one. Neither compile-bootstrap.cjs nor compile-preamble.mjs sets it, which is why the artifact column reads absent. The row can also fail for the right reason: ref is ok:marker=<version> and plain is ok:marker=absent, so plain != ref counts the row as discriminating rather than vacuous, and a marker that came back with the same value would collapse the inequality and fail.
ℹ️ A user-visible behavior fork is recorded only in the internal design doc
The consequence the commit message names — a library feature-gating on the marker silently takes its plain-Node branch inside a compiled binary — lands on whoever ships that binary, and site/content/docs/compile.mdx has the section for it. ## Runtime behavior → ### Process identity enumerates exactly which process.* values differ in an artifact (execPath, argv, argv0, title, execArgv) and does not mention process.versions.nub, so the one process value a third-party library is most likely to branch on is the one absent from the list.
Technical details
# `process.versions.nub`'s absence is not on the compile docs page
## Affected sites
- `site/content/docs/compile.mdx` `### Process identity` (under `## Runtime behavior`) — lists the
`process.*` values a compiled artifact changes; `process.versions.nub` is not among them.
- `wiki/design/compiled-executables.md:317-322` — where the decision is currently recorded. The
sibling sections there (`### No public "am I a compiled binary?" API`, the deliberate-drop list)
are also wiki-only, so this is the pattern rather than a one-off.
- `crates/nub-cli/tests/process_versions_nub.rs:78` — calls the `--node` result "the plain-Node
fingerprint a tool checking `process.versions.nub` should see", i.e. the marker is treated as a
detection surface for third-party code, not an internal.
- `site/content/blog/nub-0-3-0.mdx:57` — the marker was announced publicly as a feature.
## Required outcome
- An author reading the compile docs can learn that `process.versions.nub` is absent in an artifact
and what to do instead, without reading `wiki/`.
## Suggested approach (optional)
- One line in `### Process identity` alongside the `process.*` block. Note that `site/content/docs/`
never introduces the marker today (only the 0.3.0 blog post does), so the sentence has to name it
rather than describe it as changed.
## Open questions for the human
- Is `compile.mdx` meant to carry the deliberate-drop set at all, or is `wiki/` the intended home for
the whole class? The answer decides whether this is one line or a section, and it applies to the
`.env` and `Bun.isStandaloneExecutable` decisions the same way.ℹ️ Nitpicks
- "the fixtures reproduce 22 polyfilled globals on Node 22" appears twice —
tests/compile-augmentation/fixtures/a-version-marker.mjs:8andwiki/design/compiled-executables.md:321— and no committed output produces that number.a-global-parity.mjsprintsok:<total names>:<digest>overglobalThisplus the members of 16 builtins, which is hundreds and does not separate polyfilled from native;a-modern-apis.mjsprobes 27 named APIs and printsName=Y/n/!per entry without partitioning either. Naming the derivation (e.g. "22 ofa-modern-apis's 27 probes were polyfilled on 22.15") or dropping to a qualitative phrase would keep it from going stale in two places at once.
Claude Opus ｜ 𝕏
The harness compared the last line of stdout and nothing else, so a fixture that printed the right answer and then died would have passed. Every row now carries the process's exit status, which closes that and gives exit-code coverage across all of them at once rather than through a mechanism of its own. Audited the surface first and found no defect: process.exit(n), process.exitCode, an uncaught throw, an unhandled rejection, SIGINT (130) and SIGTERM all match `nub <file>`, stdout and stderr stay separate, stdin pipes through, and process.stdout.isTTY is true under a real pty and false through a pipe — the same as plain Node, so colour output and interactive prompts survive compiling. A compiled CLI's exit code is contract, and the launcher runs the user's Node as a child, so forwarding it is real work that could go wrong silently. The new fixture exits 7 rather than 1 because an uncaught throw already produces 1, which would still pass if the status were being invented instead of forwarded — it is therefore also the control proving the harness reads a real status.
There was a problem hiding this comment.
ℹ️ No critical issues — the new comparison is mechanically right and turns no row red. Two suggestions inline.
Reviewed changes — the one commit since the f26d5e3 review, 276f547c test(compile): compare exit status, not just output. No product code.
- Compared exit status alongside the last output line —
tests/compile-augmentation/run.shnow captures$?for each of the three columns on its own statement and appendsrc=<status>, so a launcher that loses the child's status fails a row instead of passing it. - Added a fixture that exits non-zero —
tests/compile-augmentation/fixtures/a-exit-code.mjsprints its marker and callsprocess.exit(7), with the header explaining that 7 rather than 1 is what stops an invented status from passing.
I checked whether the new dimension turns a currently-green row red, and it does not. Every pre-existing fixture exits 0 in all three columns — each ends in a console.log with no throw reaching the top, a-modern-apis and a-unflagged wrap every probe in a try/catch, and a-worker.ts discards the worker's code — while a-exit-code round-trips 7 through two structurally identical mappings: exit_code_from_status (crates/nub-core/src/node/spawn.rs:3147, whose own test at :3602 asserts 7) and the launcher's exit_code (crates/nub-launcher/src/main.rs:2811, reached from run at :178). The capture itself is right too: after an assignment whose right-hand side is a command substitution, $? is the substitution's status, and no pipe intervenes. Worth knowing for scope — tests/compile-corpus/fixtures/a-process.mjs already asserts r.status === 7 across the same launcher hop, and Windows gates that by copying the fixture (tests/windows/compile-process-contract.ps1), so what is new here is CI reach on Linux, where the corpus runs locally only.
ℹ️ Nitpicks
a-exit-codewill be reported as(vacuous on 26.5.0)and counted in thevacuoustotal: the vacuity test isplain != ref(run.sh:130), and plain Node exits 7 as well. That makes it the second fixture whose subject is compile-only — aftera-sourcemap— to land in the bucketrun.sh:13-17describes as "would pass with nub deleted", which is the opposite of true for both. Since that counter is the harness's own honesty signal, a third label for rows whose subject is the artifact column may be worth carrying.console.logfollowed immediately byprocess.exit(7)can lose the line on macOS. Node documentsprocess.stdoutpipe writes as synchronous on Windows and Linux but asynchronous on macOS, and the harness captures through a command substitution. CI is Linux-only so nothing is at risk there, but a local run on a Mac can truncate;process.exitCode = 7exercises the same launcher hop without the hazard.
Claude Opus ｜ 𝕏
| # Captured on its own line, never through the pipe — `$(cmd | tail -1)` would | ||
| # report tail's status and always be 0. |
There was a problem hiding this comment.
This script sets set -uo pipefail at :28, and under pipefail a pipeline's status is the rightmost non-zero one — so $("$PLAIN_NODE" "$entry" | tail -1) would have reported 7, not 0. Checked on the bash the harness runs under: with pipefail the old form gives rc=7, with set +o pipefail it gives rc=0. The refactor is still the better spelling, since it does not depend on pipefail staying on and it keeps the whole output in hand; only the reason is wrong.
| # Captured on its own line, never through the pipe — `$(cmd | tail -1)` would | |
| # report tail's status and always be 0. | |
| # Captured on its own statement rather than as `$(cmd | tail -1)`: that form | |
| # reports the program's status only while the `set -o pipefail` above stays on, | |
| # and keeping the whole output in a variable does not depend on it. |
| plain_out="$("$PLAIN_NODE" "$entry" 2>&1)"; plain_rc=$? | ||
| plain="$(printf '%s' "$plain_out" | tail -1) rc=$plain_rc" | ||
| ref_out="$("$NUB" "$entry" 2>&1)"; ref_rc=$? | ||
| ref="$(printf '%s' "$ref_out" | tail -1) rc=$ref_rc" |
There was a problem hiding this comment.
:135 prints ${ref:0:24} and ${got:0:24} into %-24s fields, so the new rc= suffix is the first thing truncated away. a-modern-apis emits ok: plus 27 comma-joined k=v pairs, so a row failing only on exit status shows two identical-looking columns beside a bare FAIL. Prefixing keeps the dimension visible; :102 needs the same change.
Technical details
# The compared exit status is invisible in the table it is compared through
## Affected sites
- `tests/compile-augmentation/run.sh:85`, `:87`, `:102` — ` rc=$…` is appended after the last output
line, so it occupies the tail of each column string.
- `tests/compile-augmentation/run.sh:135` — `printf '%-22s %-24s %-24s %s\n' "$name" "${ref:0:24}" "${got:0:24}" "$verdict"`.
Any column whose last line reaches 24 characters loses the suffix entirely.
- Fixtures already past it: `a-modern-apis.mjs:33` (`ok:` plus ~27 comma-joined `k=v` pairs) and
`a-sourcemap.ts`'s miss branch (`ok:unresolved:<40 chars>`). `a-global-parity.mjs` lands right on
the boundary at 19-20 characters, so it is one wider name-count away from the same fate.
## Required outcome
- A row that fails only because the exit status diverged prints enough for the operator to see that,
without re-running the fixture by hand.| plain_out="$("$PLAIN_NODE" "$entry" 2>&1)"; plain_rc=$? | |
| plain="$(printf '%s' "$plain_out" | tail -1) rc=$plain_rc" | |
| ref_out="$("$NUB" "$entry" 2>&1)"; ref_rc=$? | |
| ref="$(printf '%s' "$ref_out" | tail -1) rc=$ref_rc" | |
| plain_out="$("$PLAIN_NODE" "$entry" 2>&1)"; plain_rc=$? | |
| plain="rc=$plain_rc $(printf '%s' "$plain_out" | tail -1)" | |
| ref_out="$("$NUB" "$entry" 2>&1)"; ref_rc=$? | |
| ref="rc=$ref_rc $(printf '%s' "$ref_out" | tail -1)" |
`out="$(… | tail -1)"; rc=$?` reports TAIL's status, not the artifact's, so the `[ "$rc" = 0 ]` check next to it was decoration: a compiled binary for any of the 14 real packages that printed the right last line and then died passed for the same reason a working one did. Measured directly — a command exiting 7 reports rc=0 through that shape and rc=7 without the pipe. Status is now captured on its own line, and the failure message shows it, since it is a thing that can fail the row now. Verified by adding a fixture that prints correctly then exits 9: red with it, green without. Also stops fixtures accumulating in the work dir. The node_modules tree is reused deliberately — reinstalling a dozen native packages per run would make this unusable — but the fixtures were being copied in beside whatever was already there, so a deleted or renamed one kept running and reported a failure that no longer existed in the tree under test. That cost real time to diagnose today.
There was a problem hiding this comment.
ℹ️ No new issues in the delta — the refactor is the better spelling, but the reason it records is not true under this script's own
pipefail.
Reviewed changes — the one commit since the 276f547 review, 85bc1c73 test(compile): the corpus harness's exit-code gate could never fail. One file, no product code.
- Reset the fixture copies before each run —
tests/compile-corpus/run.shdeletesa-*.mjsandfork-child.mjsfrom the reused work dir before copying, so a renamed or deleted fixture stops being run out of a stale copy. - Split the artifact's status capture off the pipe — the run is captured into
raw,$?is taken on its own statement, and the last line comes fromprintf '%s' "$raw" | tail -1; the FAIL row now printsrc=<status>beside the wanted one.
I checked the premise rather than the diff. run.sh:11 is set -uo pipefail, | binds tighter than &&, and git show 276f547c:tests/compile-corpus/run.sh confirms rc=$? and the [ "$rc" = 0 ] gate were both already there — so the pre-image reported the artifact's status, and the gate was live. Verified on the runner's bash 5.2.21: with pipefail the old form gives rc=7, with set +o pipefail it gives rc=0. The change is therefore a no-op on the gate; it turns no row red and closes no false pass. Worth knowing for scope: :63 passes the whole string to %-24s, which only pads, so the truncation concern raised against the sibling harness does not transfer. The rm -f list is also exhaustive today — 14 a-*.mjs fixtures plus exactly one non-a- file, fork-child.mjs, which a-fork.mjs:10 forks.
ℹ️ Nitpicks
rm -f "$WORK"/a-*.mjs "$WORK"/fork-child.mjs(:35) enumerates the two fixture name shapes that exist today.rm -f "$WORK"/*.mjscovers the same hazard and needs no edit when a fixture arrives under a new prefix.- The sweep covers the fixtures but not what they produce:
bin-<name>,log-<name>andc-<name>for a removed fixture still survive in the reused work dir, and eachbin-*carries an embedded Node. Nothing stale is executed — the loop iteratesa-*.mjs— so this is disk rather than correctness.
Claude Opus ｜ 𝕏
| # Status on its own line, never through the pipe: `$(cmd | tail -1)` reports | ||
| # TAIL's status, so the rc gate below silently accepted anything — an | ||
| # artifact that printed the right last line and then died passed for the | ||
| # same reason a working one did. |
There was a problem hiding this comment.
:11 sets set -uo pipefail, and under pipefail a pipeline's status is the rightmost non-zero one — so the old out="$(… | tail -1)"; rc=$? reported the artifact's 7, not tail's 0, and the [ "$rc" = 0 ] gate below was already live. Checked on the runner's bash 5.2.21: with pipefail the old form gives rc=7, with set +o pipefail it gives rc=0. The refactor is still the better spelling because it does not depend on pipefail staying on; only the reason (and the commit subject) is wrong. The same wording is still open on the sibling harness at tests/compile-augmentation/run.sh:82-83.
| # Status on its own line, never through the pipe: `$(cmd | tail -1)` reports | |
| # TAIL's status, so the rc gate below silently accepted anything — an | |
| # artifact that printed the right last line and then died passed for the | |
| # same reason a working one did. | |
| # Status captured on its own statement rather than as `$(cmd | tail -1)`: that | |
| # form reports the program's status only while the `set -o pipefail` above stays | |
| # on, and this does not depend on it. |
Re-measured: a pure-JavaScript embed artifact is 27.9 MB shipped (the prose was right) and expands to 102.1 MB, of which the decompressed Node is 102.0. The doc said 'roughly 103 MB … 102 MB of that the decompressed Node', which put a 1 MB remainder next to it that does not exist and made the two figures read as inconsistent with each other.
`nub compile` exposed it and nothing described it. Audited the whole flag surface against the docs to find it: every compile-only flag is now covered, and the only remaining gaps are --color, --cwd and --verbose, which are global flags undocumented repo-wide rather than anything specific to this command. Verified by running rather than from the help text: a JSON file substitutes as a JavaScript expression with no massaging, the trailing newline is dropped, a missing file fails the build naming the key, and deleting the source files after the build does not change what the binary does — the value is in the bundle, so it behaves like the rest of strict compilation.
zizmor went red on a docs-only push, having passed seven times today on the same lines. Not a zizmor change: upstream released dorny/paths-filter v3.0.4 and moved the floating `v3` tag to it, so a comment reading `# v3` beside a pin of d1c1ffe0 stopped being true. Verified against the GitHub API rather than taking the linter's word: `v3` now resolves to 0e4a8c6e (v3.0.4) and d1c1ffe0 is exactly v3.0.3. The comment is corrected to say so. The pinned hash is deliberately unchanged — that is a dependency upgrade and a separate decision. This makes the comment honest about what is already pinned, which is the whole point of pinning by hash and annotating with a version.
There was a problem hiding this comment.
ℹ️ No critical issues — every claim in the new docs section checks out against the implementation. Two suggestions below.
Reviewed changes — the two commits since the 85bc1c7 review. No product code and no test code in either.
- Documented
--define-file— a new###section insite/content/docs/compile.mdx(12 added lines,:173-184), placed under## --definebetween that section's last code block and## --include. - Corrected the first-run expansion figure — one line in
wiki/design/compiled-executables.md, covered by the previous run, which submitted no review.
Nothing is anchored inline: the formatted diff renders site/content/docs/compile.mdx as (binary file or no changes), so line references below are to the working tree at 9f9d37b.
Two things I checked rather than took on trust. Every factual claim in the new section holds against read_define_files (crates/nub-cli/src/compile/mod.rs:689-722) and its two unit tests: one trailing line ending is dropped and CRLF with it, the rest is substituted verbatim, a read failure names the key, and the file is read once at :104 before bundling so deleting it afterwards changes nothing. The commit message's broader claim also holds — the Compile variant's 25 flags (crates/nub-cli/src/cli.rs:1029-1190) each now have a heading on that page.
ℹ️ The section motivates with a prompt but only works the value shape that needs no quoting
The lead sentence offers "a prompt, a schema, a table of model definitions" as what outgrows a command line, and the only worked example is JSON — the one shape that is already an expression, with the reassurance that it "usually needs nothing done to it". A reader pointing --define-file PROMPT=./prompt.txt at raw prose has been told the requirement in the abstract and shown only the case where it does not bite.
Technical details
# The `--define-file` section shows only the value shape that needs no massaging
## Affected sites
- `site/content/docs/compile.mdx:176` — motivates the flag with "a prompt, a schema, a table of
model definitions", then states the requirement once, in the abstract: "one JavaScript expression".
- `site/content/docs/compile.mdx:182` — the only worked value is `{"name":"a","dims":512}`, followed
by "so a JSON file usually needs nothing done to it". Of the three motivating cases, the two that
need work as plain files (a prompt, a table written as prose or CSV) are the ones not shown.
- `crates/nub-cli/src/compile/mod.rs:704-712` — a file that reads fine but is not UTF-8 is refused
with its own error naming the offset and pointing at `--include`. "The build fails naming the key
if the file cannot be read" (`:182`) does not describe that case.
## Required outcome
- A reader whose value is text rather than JSON learns from this section that the file holds a
quoted string, before they run the build.
## Suggested approach (optional)
- One added sentence covering both: a text value ships as a string literal, so a prose `prompt.txt`
has to become a quoted (for example JSON-encoded) string, and the file is read as UTF-8 either way.ℹ️ Nitpicks
site/content/docs/compile.mdx:182opens a paragraph with inline code, whichPROSE.md:33states as absolute ("No exceptions", docs included). Across the whole pagegrep '^[^]'matches exactly two lines — this new one and the pre-existing:439— so the delta adds one of the only two. "Here./models.jsonholds …" satisfies both that rule and the capital-letter one beside it.- Precedence between the two flags is unstated.
--define-filevalues are appended after the argv defines (crates/nub-cli/src/compile/mod.rs:101-105, into theFxIndexMapbuilt atbundle.rs:3585), so a key given both ways takes the file's value. The## --definesection already documents its own precedence over the three automatic defines (:159), which makes this the missing half of the same sentence.
Claude Opus ｜ 𝕏
nub compileturns a JS/TS entry into a standalone executable: a Rust launcher carrying an embedded payload, which extracts real files into a content-addressed cache and runs them under stock Node.Closes #67
Refs #567
The request in #67 suggested Node's SEA. This uses a Rust launcher with an embedded payload instead: SEA cannot carry a per-file executable bit or relocate a native package closure, both of which the addon cases below need.
What this adds
--smol,--platformcross-compilation, and the extraction cache..nodeaddon survives bundling, its owning package and that package's production dependency closure are relocated under__nub_native/<hash>/, preserving real install geometry as ordinary files. This is what makessharpwork: itslibvipsshared library lives in a different package, and the addon resolves it by relative path at load time.Verified
Every claim below is a run, not an inference. Platform is named where it matters.
nub-coreembed-runtime, the launcher cache chain, native islands, the process-topology suite, and the embedded round-trip. Eight distinct defects were fixed to get there, none of them reproducible off Windows. A stockC:\carries anINHERIT_ONLYACE that the DACL walk counted as an effective grant, so no cache base was usable on any Windows machine.fs::canonicalizereturns a verbatim path that Rolldown cannot resolve as a specifier and Node cannot resolve as a module — the latter is nodejs/node#60435, open upstream.clapbuilds its derived command tree at startup and overflowed Windows' 1 MiB main-thread stack before any subcommand ran. And whichever subsystem created the cache root first decided the security posture of everything beneath it, so an explicitly setXDG_CACHE_HOMEwas silently ignored for the runtime and the tree landed on another drive.sharpcompiles and runs with the source tree andnode_modulesdeleted, its transitivelibvipsresolving from the extracted island. This previously depended onllvm-stripbeing installed: with only Applestripon PATH, the embedded Node lost its exportednapi_*symbols and every addon failed todlopen. Since release builds have nollvm-strip, that shape shipped broken; the strip step is now guarded.\\?\) cache path. Node cannot resolve a module through one at all: it reads the root as the volume device, libuv failsEISDIR, and the error prints with the prefix stripped, so it surfaced as the bafflinglstat 'C:'. That is nodejs/node#60435, open with a stalled fix, so the spelling is corrected on our side.clusterworkers see[artifact, entry]and nothing else. They previously also saw nub's private flags and a duplicated entry, because the entry chunk evaluatesnode:clusterbefore the preamble runs and Node'sprimary.jscapturesforkinto a module-local const at that moment.require()of an ES module resolves to the namespace. It previously resolved toundefined, silently, on every Node version; the cause is upstream in rolldown, where a concise arrow body is treated as a discarded expression statement.Test coverage this also fixes
Four tests in this area passed while proving nothing, and are now fixed: two Windows reparse tests that always took their skip branch, and both embedded round-trip fixtures — those were types-only TypeScript, which Node strips natively since 23.6, so they printed their success marker even in a run where extraction had failed outright. They now use a non-erasable
enum, so plain Node fails and only a real transpile succeeds.CI ran two of roughly 221 compile integration tests, by name, both gated to a single Node version — and the second was unreachable regardless, since the step aborts at the first failure and the other test ran ahead of it. The Linux leg now runs the whole target. Separately, the launcher's cache-directory probe chain ran on ubuntu only, which is precisely why a defect that made every Windows cache base unusable was invisible; it now runs on the platforms it targets. Every Windows defect this PR fixes was invisible on a developer machine and surfaced only on a real runner.
Known limitations
Documented in
compile.mdx, with--externalas the remedy where one applies.bindings, or through a specifier assembled at runtime, is not followed. Only static requires are.createRequire(...)result is an ordinary call the bundler cannot rewrite; use a static import.require()normalization is a stopgap until the rolldown fix lands upstream and the pin moves.