From da6370baf8f729397f3568d57d18d72693d340be Mon Sep 17 00:00:00 2001 From: Norin Lavaee Date: Thu, 6 Aug 2026 12:03:30 -0700 Subject: [PATCH 1/2] test: build the native binding when it is missing, warn when it is stale 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 --- DEV_SETUP.md | 33 ++++++- test/global-setup-natives.ts | 176 +++++++++++++++++++++++++++++++++++ vitest.config.ts | 14 +++ 3 files changed, 218 insertions(+), 5 deletions(-) create mode 100644 test/global-setup-natives.ts diff --git a/DEV_SETUP.md b/DEV_SETUP.md index 93c6740f95..63daacb53f 100644 --- a/DEV_SETUP.md +++ b/DEV_SETUP.md @@ -28,13 +28,36 @@ npm run build --workspace=@bastani/atomic-natives The natives build is a required one-time step (and again after pulling changes to `crates/` or `packages/natives/`). `npm ci --ignore-scripts` deliberately skips lifecycle scripts, and the workspace natives package has no install hook anyway — -only published releases ship prebuilt binaries. Without the compiled -`packages/natives/native/*.node`, the CLI still runs but silently degrades: -`pty:true` bash falls back to pipes, native grep/find/tree-sitter block -resolution fall back to slower JS paths, and several `packages/coding-agent` -tests fail (`bash-pty-native`, `search-tool-*`, `hashline-tools`). CI builds the +only published releases ship prebuilt binaries. + +**`vitest` now builds it for you when it is missing.** The `globalSetup` in +`test/global-setup-natives.ts` checks for `packages/natives/native/*.node` +before collecting any file: present and current, it returns after one stat and +costs nothing; missing, it prints what it is doing and runs the build; older +than the Rust sources, it warns and runs anyway, because `git checkout` rewrites +mtimes and blocking a suite on that evidence is worse than one build too few. +You still need a Rust toolchain — without cargo it fails with that prerequisite +rather than a compile error. + +Running the CLI is not covered by that, so build it yourself before using the +agent from a fresh checkout. Without the compiled binding, `pty:true` bash falls +back to pipes and native grep/find/tree-sitter block resolution fall back to +slower JS paths. + +What is **not** a graceful degradation is the test suites. Since the in-process +subagent runner landed, `packages/subagents` reaches the Rust control plane +through a *static* import, so a missing binding throws while the module graph is +still loading and takes roughly twenty root unit and integration files with it — +not just `bash-pty-native`, `search-tool-*`, and `hashline-tools`. The errors +name whatever imported the extension, such as `workflow-stage-bundled-resources`, +so the failure reads like a regression in an unrelated subsystem. CI builds the module explicitly for the same reason (see `.github/workflows/test.yml`). +Note that the generated napi-rs loader's own miss message suggests removing +`package-lock.json` and `node_modules` and re-running `npm i`. That advice does +not apply here: with `--ignore-scripts`, reinstalling never produces the +binding. `npm run build --workspace=@bastani/atomic-natives` is the fix. + The committed `.npmrc` applies a three-day minimum release age to anything you add with `npm install`, and pins exact versions. `package-lock.json` is the only lockfile. diff --git a/test/global-setup-natives.ts b/test/global-setup-natives.ts new file mode 100644 index 0000000000..ec56c4403e --- /dev/null +++ b/test/global-setup-natives.ts @@ -0,0 +1,176 @@ +import { spawnSync } from "node:child_process"; +import { existsSync, readdirSync, statSync } from "node:fs"; +import { join } from "node:path"; + +/** + * Make `@bastani/atomic-natives` present before the suites run, and say so out + * loud when it is not. + * + * `npm ci --ignore-scripts` is the mandated install (AGENTS.md) and the natives + * workspace has no install hook, so a fresh checkout or a new git worktree has + * no `.node` at all. Since the in-process subagent runner landed, + * `packages/subagents` reaches the Rust control plane through a *static* + * import, so a missing binding is not a graceful degradation: 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. + * + * The generated napi-rs loader makes that worse. Its own miss message is: + * + * Cannot find native binding. npm has a bug related to optional dependencies + * ... Please try `npm i` again after removing both package-lock.json and + * node_modules directory. + * + * That advice is wrong here. Reinstalling under `--ignore-scripts` never + * produces the binding, so a developer who follows it loops. `native/index.js` + * is generated (`@ts-nocheck`) and would lose any hand edit at the next + * `napi artifacts`, so the correct instruction has to live here. + * + * 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 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. 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 here is a + * timestamp comparison rather than their embedded version sentinel, because our + * `.node` carries no sentinel to compare against. + * + * Cost model: one `existsSync` plus a 27-file stat walk (~4 ms) on the happy + * path. CI builds the binding in an explicit step before invoking vitest, and a + * warm worktree already has it, so neither pays for the build. + */ + +const REPO_ROOT = join(import.meta.dirname, ".."); +const NATIVE_DIR = join(REPO_ROOT, "packages", "natives", "native"); +const BUILD_COMMAND = "npm run build --workspace=@bastani/atomic-natives"; +/** Sources whose edit invalidates a compiled binding. */ +const SOURCE_ROOTS = [join(REPO_ROOT, "crates"), join(REPO_ROOT, "packages", "natives", "src")]; + +function note(lines: readonly string[]): void { + process.stderr.write(`\n${lines.join("\n")}\n\n`); +} + +/** Every compiled binding present, newest first. */ +function compiledBindings(): string[] { + if (!existsSync(NATIVE_DIR)) return []; + try { + return readdirSync(NATIVE_DIR) + .filter((entry) => entry.endsWith(".node")) + .map((entry) => join(NATIVE_DIR, entry)); + } catch { + return []; + } +} + +/** Newest mtime across Rust sources, or 0 when none can be read. */ +function newestSourceMtime(): number { + let newest = 0; + const visit = (directory: string): void => { + let entries: string[]; + try { + entries = readdirSync(directory); + } catch { + return; + } + for (const entry of entries) { + const path = join(directory, entry); + let stats: ReturnType; + try { + stats = statSync(path); + } catch { + continue; + } + if (stats.isDirectory()) { + if (entry !== "target" && entry !== "node_modules") visit(path); + continue; + } + if (entry.endsWith(".rs") || entry === "Cargo.toml") newest = Math.max(newest, stats.mtimeMs); + } + }; + for (const root of SOURCE_ROOTS) visit(root); + return newest; +} + +function buildNatives(): { ok: true } | { ok: false; reason: string } { + const result = spawnSync("npm", ["run", "build", "--workspace=@bastani/atomic-natives"], { + cwd: REPO_ROOT, + stdio: "inherit", + shell: process.platform === "win32", + }); + if (result.error !== undefined) return { ok: false, reason: result.error.message }; + if (result.status !== 0) return { ok: false, reason: `the build exited with status ${result.status}` }; + return { ok: true }; +} + +export default function setup(): void { + const existing = compiledBindings(); + + if (existing.length > 0) { + // Stale is a warning, never a rebuild and never a failure. A timestamp is + // weaker evidence than a version sentinel -- `git checkout` rewrites + // mtimes, so this can cry wolf after a branch switch -- and blocking a + // suite on weak evidence is worse than running one build too few. + const newestSource = newestSourceMtime(); + const stale = existing.filter((binding) => { + try { + return statSync(binding).mtimeMs < newestSource; + } catch { + return false; + } + }); + if (stale.length > 0) { + note([ + "WARNING: the compiled native binding is older than the Rust sources.", + ...stale.map((binding) => ` stale: ${binding}`), + "", + "Tests will run against the binding already on disk. If results look", + "impossible, rebuild first:", + ` ${BUILD_COMMAND}`, + ]); + } + return; + } + + note([ + "@bastani/atomic-natives has no compiled binding, so it is being built now.", + ` looked in: ${NATIVE_DIR}`, + "", + "Without it packages/subagents fails at import and takes roughly twenty", + "unrelated unit and integration files down with it, naming the importer", + "rather than the binding.", + "", + "This happens once per checkout or git worktree, and takes about 35 seconds.", + ` ${BUILD_COMMAND}`, + ]); + + const built = buildNatives(); + if (!built.ok) { + throw new Error( + [ + `Could not build @bastani/atomic-natives: ${built.reason}.`, + "", + `Looked for a compiled binding in: ${NATIVE_DIR}`, + "", + "The suites cannot run without it: packages/subagents imports the Rust", + "control plane statically, so the bundled extension fails at import.", + "", + "This build needs a stable Rust toolchain with cargo (https://rustup.rs).", + "The generated napi-rs loader will instead suggest reinstalling with npm;", + "that cannot work here, because the mandated `npm ci --ignore-scripts`", + "never runs the build.", + "", + `Once cargo is available, run: ${BUILD_COMMAND}`, + ].join("\n"), + ); + } + + if (compiledBindings().length === 0) { + throw new Error( + `${BUILD_COMMAND} reported success but left no .node file in ${NATIVE_DIR}. Run it directly to see why.`, + ); + } +} diff --git a/vitest.config.ts b/vitest.config.ts index ce65557ea2..c994bae9a7 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -11,6 +11,19 @@ export { TEST_TIMEOUT_MS }; */ const setupFiles = ["./test/setup-workflow-durability.ts"]; +/** + * Runs once per project, before any file is collected. It builds + * `@bastani/atomic-natives` only when no compiled binding exists, because a + * missing binding no longer degrades gracefully: `packages/subagents` imports + * the Rust control plane statically, so the bundled extension throws during + * module loading and takes roughly twenty unrelated files down with it under + * errors that name the importer rather than the binding. + * + * On the happy path this is a single `existsSync`, so CI — which builds the + * binding in an explicit step first — and any warm worktree pay nothing. + */ +const globalSetup = ["./test/global-setup-natives.ts"]; + const project = (name: string, directory: string) => ({ resolve: { alias: sharedAliases }, test: { @@ -21,6 +34,7 @@ const project = (name: string, directory: string) => ({ include: [`${directory}/**/*.test.ts`], exclude: ["**/node_modules/**"], setupFiles, + globalSetup, testTimeout: TEST_TIMEOUT_MS, hookTimeout: TEST_TIMEOUT_MS, }, From 046e7258825d66417afc8824ea7278cf2382f8f1 Mon Sep 17 00:00:00 2001 From: Norin Lavaee Date: Thu, 6 Aug 2026 13:33:22 -0700 Subject: [PATCH 2/2] test(natives): detect a binding by loading it, and use the runtime helpers 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 --- test/global-setup-natives.ts | 117 +++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 55 deletions(-) diff --git a/test/global-setup-natives.ts b/test/global-setup-natives.ts index ec56c4403e..21aba54e8c 100644 --- a/test/global-setup-natives.ts +++ b/test/global-setup-natives.ts @@ -1,51 +1,43 @@ -import { spawnSync } from "node:child_process"; import { existsSync, readdirSync, statSync } from "node:fs"; +import { createRequire } from "node:module"; import { join } from "node:path"; +import { spawnSyncCollect } from "./helpers/runtime.js"; /** - * Make `@bastani/atomic-natives` present before the suites run, and say so out + * Make `@bastani/atomic-natives` loadable before the suites run, and say so out * loud when it is not. * * `npm ci --ignore-scripts` is the mandated install (AGENTS.md) and the natives * workspace has no install hook, so a fresh checkout or a new git worktree has - * no `.node` at all. Since the in-process subagent runner landed, + * no compiled binding. Since the in-process subagent runner landed, * `packages/subagents` reaches the Rust control plane through a *static* - * import, so a missing binding is not a graceful degradation: the bundled - * extension throws while the module graph is still loading and takes roughly - * twenty root unit and integration files down with it. + * import, so that is not a graceful degradation: 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. * - * The generated napi-rs loader makes that worse. Its own miss message is: - * - * Cannot find native binding. npm has a bug related to optional dependencies - * ... Please try `npm i` again after removing both package-lock.json and - * node_modules directory. - * - * That advice is wrong here. Reinstalling under `--ignore-scripts` never - * produces the binding, so a developer who follows it loops. `native/index.js` - * is generated (`@ts-nocheck`) and would lose any hand edit at the next - * `napi artifacts`, so the correct instruction has to live here. + * The generated napi-rs loader makes that worse. Its own miss message says to + * remove `package-lock.json` and `node_modules` and re-run `npm i`. That advice + * is wrong here: under `--ignore-scripts`, reinstalling never produces the + * binding, so a developer who follows it 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 here. * * 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 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. 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 here is a - * timestamp comparison rather than their embedded version sentinel, because our - * `.node` carries no sentinel to compare against. - * - * Cost model: one `existsSync` plus a 27-file stat walk (~4 ms) on the happy - * path. CI builds the binding in an explicit step before invoking vitest, and a - * warm worktree already has it, so neither pays for the build. + * loader-state.js`: report what was 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 while a rebuild is pending. 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. */ const REPO_ROOT = join(import.meta.dirname, ".."); const NATIVE_DIR = join(REPO_ROOT, "packages", "natives", "native"); +const NATIVE_ENTRY = join(NATIVE_DIR, "index.js"); const BUILD_COMMAND = "npm run build --workspace=@bastani/atomic-natives"; /** Sources whose edit invalidates a compiled binding. */ const SOURCE_ROOTS = [join(REPO_ROOT, "crates"), join(REPO_ROOT, "packages", "natives", "src")]; @@ -54,15 +46,24 @@ function note(lines: readonly string[]): void { process.stderr.write(`\n${lines.join("\n")}\n\n`); } -/** Every compiled binding present, newest first. */ -function compiledBindings(): string[] { - if (!existsSync(NATIVE_DIR)) return []; +/** + * Whether a binding usable *by this host* is present. + * + * Deliberately a load attempt rather than a filename scan. napi-rs resolves a + * platform-arch-libc triple through roughly seven hundred lines that also cover + * musl detection, Android, and WASI; a scan that accepted any `.node` would skip + * the build when the directory holds only a binding for another platform — a + * real state after copying a native directory between checkouts or unpacking + * release artifacts. Requiring the generated entrypoint asks the exact question + * the suites will ask, so it cannot drift from the loader. + */ +function bindingLoads(): boolean { + if (!existsSync(NATIVE_ENTRY)) return false; try { - return readdirSync(NATIVE_DIR) - .filter((entry) => entry.endsWith(".node")) - .map((entry) => join(NATIVE_DIR, entry)); + createRequire(import.meta.url)(NATIVE_ENTRY); + return true; } catch { - return []; + return false; } } @@ -95,27 +96,25 @@ function newestSourceMtime(): number { return newest; } -function buildNatives(): { ok: true } | { ok: false; reason: string } { - const result = spawnSync("npm", ["run", "build", "--workspace=@bastani/atomic-natives"], { - cwd: REPO_ROOT, - stdio: "inherit", - shell: process.platform === "win32", - }); - if (result.error !== undefined) return { ok: false, reason: result.error.message }; - if (result.status !== 0) return { ok: false, reason: `the build exited with status ${result.status}` }; - return { ok: true }; +/** Compiled bindings on disk, for staleness reporting only. */ +function bindingFiles(): string[] { + try { + return readdirSync(NATIVE_DIR) + .filter((entry) => entry.endsWith(".node")) + .map((entry) => join(NATIVE_DIR, entry)); + } catch { + return []; + } } export default function setup(): void { - const existing = compiledBindings(); - - if (existing.length > 0) { + if (bindingLoads()) { // Stale is a warning, never a rebuild and never a failure. A timestamp is // weaker evidence than a version sentinel -- `git checkout` rewrites // mtimes, so this can cry wolf after a branch switch -- and blocking a // suite on weak evidence is worse than running one build too few. const newestSource = newestSourceMtime(); - const stale = existing.filter((binding) => { + const stale = bindingFiles().filter((binding) => { try { return statSync(binding).mtimeMs < newestSource; } catch { @@ -135,9 +134,13 @@ export default function setup(): void { return; } + const present = bindingFiles(); note([ - "@bastani/atomic-natives has no compiled binding, so it is being built now.", + "@bastani/atomic-natives has no binding this host can load, so it is being built now.", ` looked in: ${NATIVE_DIR}`, + ...(present.length > 0 + ? [" present but not loadable here:", ...present.map((binding) => ` ${binding}`)] + : [" no .node files found"]), "", "Without it packages/subagents fails at import and takes roughly twenty", "unrelated unit and integration files down with it, naming the importer", @@ -147,13 +150,17 @@ export default function setup(): void { ` ${BUILD_COMMAND}`, ]); - const built = buildNatives(); - if (!built.ok) { + const result = spawnSyncCollect(["npm", "run", "build", "--workspace=@bastani/atomic-natives"], { cwd: REPO_ROOT }); + + if (!result.success) { throw new Error( [ - `Could not build @bastani/atomic-natives: ${built.reason}.`, + `Could not build @bastani/atomic-natives (exit ${result.exitCode}).`, + "", + `Looked for a loadable binding in: ${NATIVE_DIR}`, + ...(present.length > 0 ? ["Present but not loadable on this host:", ...present.map((b) => ` ${b}`)] : []), "", - `Looked for a compiled binding in: ${NATIVE_DIR}`, + result.stderr.toString().trim().split("\n").slice(-12).join("\n"), "", "The suites cannot run without it: packages/subagents imports the Rust", "control plane statically, so the bundled extension fails at import.", @@ -168,9 +175,9 @@ export default function setup(): void { ); } - if (compiledBindings().length === 0) { + if (!bindingLoads()) { throw new Error( - `${BUILD_COMMAND} reported success but left no .node file in ${NATIVE_DIR}. Run it directly to see why.`, + `${BUILD_COMMAND} reported success but produced no binding this host can load in ${NATIVE_DIR}. Run it directly to see why.`, ); } }