test: build the native binding when it is missing, warn when it is stale - #2224
Merged
Conversation
A fresh checkout or new git worktree has no compiled binding, because `npm ci --ignore-scripts` is the mandated install and the natives workspace has no install hook. Since the in-process subagent runner landed that is not a graceful degradation: packages/subagents reaches the Rust control plane through a STATIC import, so the bundled extension throws while the module graph is still loading and takes roughly twenty root unit and integration files down with it. The errors name whatever imported the extension -- workflow-stage-bundled-resources, the in-process runner suites -- rather than the missing binding, so the failure reads like a regression in an unrelated subsystem. It is convincing enough that it was twice diagnosed as a broken main branch during this work. The generated napi-rs loader compounds it. Its miss message says to remove package-lock.json and node_modules and re-run `npm i`, which cannot work here: with --ignore-scripts, reinstalling never produces the binding, so following the advice loops. native/index.js is generated (@ts-nocheck) and would lose a hand edit at the next `napi artifacts`, so the correct instruction has to live at test setup. A vitest globalSetup now runs before any file is collected: present and current -> returns after one stat; CI, which builds in an explicit step first, and any warm worktree pay nothing missing -> says what it is doing and builds (~35s, once per worktree) older than crates/ -> warns and runs anyway build failed -> fails with the Rust prerequisite, the paths it searched, and the command that actually works Staleness only warns. `git checkout` rewrites mtimes, so a timestamp is weaker evidence than a version sentinel, and blocking a suite on weak evidence is worse than running one build too few. Shape borrowed from can1357/oh-my-pi's packages/natives/native/ loader-state.js, which solves the same problem for its runtime loader: report every candidate tried, give the exact command for the context you are in, and let a dev tree keep running on a stale binding rather than hard-failing. Two deliberate differences: this runs at test setup rather than load time, so it can repair the missing case instead of only describing it; and staleness is a timestamp comparison because our .node carries no embedded version sentinel to compare against. Verified on all four paths: present+fresh (1.98s, silent), stale (warns, suite still passes), missing (builds, 7 tests pass in the file that used to die at import), and missing with cargo off PATH (fails naming rustup and the build command). DEV_SETUP.md claimed a missing binding "silently degrades" and failed "several packages/coding-agent tests". That predates the static import and is corrected here. No package changelog entry: per AGENTS.md this is repository tooling and does not change shipped package behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lpers Both points from greptile's review of #2224. compiledBindings() accepted any `.node` filename, so the build was skipped when the directory held only a binding for another platform, architecture, or libc -- a real state after copying a native directory between checkouts or unpacking release artifacts. On such a tree the suites then failed exactly the way this setup exists to prevent. Presence is now a load attempt against the generated entrypoint rather than a filename scan. napi-rs resolves its platform-arch-libc triple through roughly seven hundred lines that also cover musl detection, Android and WASI; reimplementing a subset here would drift from the loader. Requiring the entrypoint asks the exact question the suites will ask. Verified by renaming the host binding to a foreign triple: the old check skipped the build, the new one reports "present but not loadable here" and rebuilds. The failure and build notices now list the bindings that are present but unusable, so a wrong-platform tree is diagnosable from the message rather than from a directory listing. The build also moves from a direct node:child_process spawnSync to spawnSyncCollect from test/helpers/runtime.ts, per AGENTS.md. That helper exists for this exact trap: Node returns `status` where the suites expect `exitCode`, so a raw port silently compares against undefined. Its captured stderr is now surfaced in the failure message too. The remaining node:fs use is presence and mtime inspection, which the helper module does not wrap and AGENTS.md does not list among the runtime traps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
flora131
added a commit
that referenced
this pull request
Aug 7, 2026
The natives globalSetup I added in #2224 turned every Linux `suites` run red, including on main. Each run passes all ~5960 tests on both retry attempts and then dies with `exit code 139` -- SIGSEGV during exit. main was green at ad21551 and red at the very next run, 6546900, which is #2224. The only other commit in that range is #2225, which changes timeout numbers, a contract test and docs, and cannot segfault. The sole new executable code is test/global-setup-natives.ts. Cause: `bindingLoads()` called `createRequire(...)(NATIVE_ENTRY)`, and globalSetup runs in vitest's ORCHESTRATOR -- the process owning the worker pool. That dlopened the NAPI addon into a process that otherwise never touches it; workers load it on demand in their own processes. The addon carries #2205's Rust control plane and its Tokio runtime, so on glibc Linux its destructors run at exit alongside pool teardown and the process dies after the suite has already succeeded. The probe now runs in a child: `node -e "require(<entry>)"`, exit 0 means loadable. That keeps every property the check was chosen for -- a real load attempt rather than a filename scan, so it cannot drift from napi-rs's ~700 lines of platform-arch-libc resolution, and a foreign-platform binding still triggers a rebuild -- while the orchestrator never loads the addon. It costs one short spawn on a path that already spawns a Rust build when the binding is missing. Verified locally on all three paths: warm (2.9s, silent), foreign binding present (reports "present but not loadable here" and rebuilds), and full unit suite 625 files / 5957 tests, exit 0. I could not reproduce the segfault locally because I only ever ran this suite on macOS, which is also why #2224 shipped with it. The proof is this PR's own Linux `suites` job. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A fresh checkout or a new git worktree has no compiled native binding, and the resulting failure names the wrong subsystem. It was convincing enough that I twice diagnosed it as a broken
mainwhile working in this repo.The trap
npm ci --ignore-scriptsis the mandated install (AGENTS.md) and the natives workspace has no install hook, so nothing buildspackages/natives/native/*.node.Since the in-process subagent runner landed,
packages/subagentsreaches the Rust control plane through a static import. A missing binding therefore throws while the module graph is still loading and takes roughly twenty root unit and integration files with it..github/workflows/test.ymlalready says so:The errors name whatever imported the extension —
workflow-stage-bundled-resources, the in-process runner suites — not the missing.node. So it reads like a regression somewhere else entirely.The generated napi-rs loader makes it worse:
That advice cannot work here. With
--ignore-scripts, reinstalling never produces the binding, so following it loops.native/index.jsis generated (@ts-nocheck) and would lose a hand edit at the nextnapi artifacts, so the correct instruction has to live at test setup.What this adds
A vitest
globalSetupthat runs before any file is collected:crates/Staleness only warns.
git checkoutrewrites mtimes, so a timestamp is weaker evidence than a version sentinel, and blocking a suite on weak evidence is worse than running one build too few.Prior art
Shape borrowed from
can1357/oh-my-pi'spackages/natives/native/loader-state.js, which solves the same problem for its runtime loader: report every candidate that was tried, give the exact command for the context you are actually in, and let a dev tree keep running on a stale binding rather than hard-failing while a rebuild is pending (isWorkspaceLoad).Two deliberate differences:
.nodecarries no embedded version sentinel. oh-my-pi embeds__piNativesV<version>and uses it to distinguish disk stale ("reinstall") from process stale ("restart — reinstalling changes nothing"). Adding an equivalent sentinel tocrates/atomic-nativeswould be a strictly better signal than mtimes and is worth doing separately.Verification
All four paths exercised directly:
npm run check— exit 0npm run test:unit— 617 files, 5824 passed, 2 skippednpm run test:ci-contracts— 6 files, 41 tests, exit 0Also
DEV_SETUP.mdclaimed a missing binding "silently degrades" and failed "severalpackages/coding-agenttests (bash-pty-native,search-tool-*,hashline-tools)". That predates the static import and is corrected here — the CLI does still degrade gracefully, but the suites do not.No package changelog entry: per
AGENTS.md, repository tooling is infrastructure-level and does not change shipped package behaviour.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Greptile Summary
The native test setup now checks whether the generated loader can load a binding for the current host and rebuilds the binding when it cannot. An isolated test with an unusable native artifact confirmed that setup rebuilds the host binding before the CI test project runs successfully.
Confidence Score: 5/5
No blocking failure remains.
The host-binding recovery path was exercised with an invalid native artifact and the configured CI test project completed after rebuilding a usable binding.
What T-Rex did
Reviews (2): Last reviewed commit: "test(natives): detect a binding by loadi..." | Re-trigger Greptile