Repository navigation
feat(embed): let hosts wrap the Node runtime, not just select one - #1086
Conversation
Extends the embedder Node hook (#1079) so a host can describe a Node *invocation* rather than a bin dir, covering wrappers — instrumenting runtimes, transpiling loaders, sandboxes — not just version managers. - EmbedderRuntime builder with selector()/wrapper() constructors. The three spawn signals are now distinct: bin_dir (PATH), node_program (NODE + spawn program), node_execpath (npm_node_execpath, the real binary node-gyp reads). selector() reproduces the old node_bin_dir. - Lifecycle-script env contribution via env_append/env_set; NODE_OPTIONS appends over the user's nodeOptions by default (env_set replaces). - set_embedder_runtime process-global so every spawn path is covered; per-call InstallOptions.runtime replaces it. Honored regardless of runtime_switching. - Version supplied, not probed; engines probes node_execpath lazily. - exec/run decoupled from process cwd (explicit base_dir) and node gains a supervised-spawn variant, so embed::{run,exec,dlx,node} drive the run-class commands in process, each rooted at project_dir. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR replaces Node binary-directory overrides with ChangesEmbedder Runtime Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant EmbedAPI
participant Runtime
participant CommandRunner
participant NodeProcess
Host->>EmbedAPI: configure runtime and invoke command
EmbedAPI->>Runtime: resolve project runtime
Runtime->>CommandRunner: provide Node paths and environment
CommandRunner->>NodeProcess: spawn with base_dir, PATH, and env
NodeProcess-->>Host: return exit code
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryExtends embedded Node execution with wrapper-aware runtime configuration and explicit project roots.
Confidence Score: 5/5The PR appears safe to merge with no remaining blocking failures related to the previous review threads. The prior project-root auto-install and late runtime-registration failures are addressed, and the existing dlx cwd concern is already covered by the displayed review feedback. Important Files Changed
Reviews (5): Last reviewed commit: "fix(embed): seed the isolation scope in ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Extends aube’s embed API so hosts can describe a full Node invocation (including wrappers/shims and env contributions), and threads that runtime through all Node spawn paths (install lifecycle scripts, run/exec/dlx/node) while avoiding process-cwd races for in-process embedding entry points.
Changes:
- Introduces
EmbedderRuntimewithselector(...)andwrapper(...).real_node(...).env_append/env_set(...), and plumbsnode_programvsnode_execpaththroughRuntimeContext. - Adds process-wide runtime registration (
set_embedder_runtime) and per-install override seeding, plus lazy version probing vianode_execpath. - Adds in-process embed helpers
embed::{run, exec, dlx, node}and updates command implementations to accept an explicit base directory to avoid cwd contention.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/embedding/rust.md | Documents the new embedder Node runtime model and new in-process run/exec/dlx/node APIs. |
| crates/aube/src/runtime.rs | Implements EmbedderRuntime, runtime seeding, env merge logic, and node_program/node_execpath split. |
| crates/aube/src/engines.rs | Probes Node version via the active runtime’s node_execpath when available. |
| crates/aube/src/embed.rs | Exposes embed-facing runtime types and adds in-process run/exec/dlx/node entry points. |
| crates/aube/src/commands/script_settings.rs | Folds embedder NODE_OPTIONS into resolved nodeOptions and passes execpath/extra env into script settings. |
| crates/aube/src/commands/runtime.rs | Updates runtime reporting to use node_program naming. |
| crates/aube/src/commands/run.rs | Adds run_script_in / base-dir plumbing to avoid process-cwd races for embedding. |
| crates/aube/src/commands/node.rs | Applies child env vars to aube node and adds supervised spawn variant for in-process embedding. |
| crates/aube/src/commands/install/mod.rs | Seeds embedder runtime into the install scope before runtime resolution. |
| crates/aube/src/commands/install/args.rs | Renames embedder install option from embedder_node_bin_dir to embedder_runtime. |
| crates/aube/src/commands/exec.rs | Adds run_in with explicit base dir and enables Default for args construction. |
| crates/aube/src/commands/doctor.rs | Updates doctor output to use node_program naming. |
| crates/aube/src/commands/dlx.rs | Adds run_in with explicit base dir and enables Default for args construction. |
| crates/aube/src/commands/deploy/mod.rs | Updates deploy install options for renamed embedder runtime field. |
| crates/aube/src/commands/ci.rs | Updates CI install options for renamed embedder runtime field. |
| crates/aube/src/commands/add/mod.rs | Updates embed-facing add options to accept EmbedderRuntime. |
| crates/aube-scripts/src/lib.rs | Splits NODE vs npm_node_execpath in script jail env stamping and supports extra embedder env. |
| crates/aube-node/src/lib.rs | Updates Node-API host install options to the new runtime field. |
| crates/aube-ffi/src/lib.rs | Updates FFI host install options to the new runtime field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn child_env_vars() -> Vec<(std::ffi::OsString, std::ffi::OsString)> { | ||
| let ctx = current(); | ||
| let execpath = ctx | ||
| .as_ref() | ||
| .and_then(|c| c.node_execpath.clone().or_else(|| c.node_program.clone())) | ||
| .or_else(aube_runtime::node_on_path); |
| // Fold an embedder's `NODE_OPTIONS` contribution over the resolved | ||
| // `nodeOptions` setting (append by default, so a wrapper's `--import` | ||
| // preload adds to the user's value rather than replacing it). | ||
| let node_options = crate::runtime::merge_node_options( | ||
| aube_settings::resolved::node_options(ctx).and_then(non_empty_string), | ||
| ); |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aube/src/commands/run.rs (1)
450-479: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPass the resolved project directory into
ensure_installed.ensure_installedhas no directory parameter and starts fromcrate::dirs::cwd(), while these embedding entry points resolvecwd/effective_cwdfrom an explicitbase_dir. Those calls can therefore evaluate auto-install freshness against the wrong tree; thread the already-resolved project directory into the helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aube/src/commands/run.rs` around lines 450 - 479, The resolved project directory is not passed to ensure_installed, causing freshness checks to use the process cwd instead of the requested project tree. Update ensure_installed and every affected caller to accept and use the resolved directory, including the run.rs flow around load_manifest and the exec.rs call site at lines 120-127; pass cwd/effective_cwd rather than recomputing it from crate::dirs::cwd().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aube/src/commands/node.rs`:
- Around line 64-92: Update run_spawn to await
crate::process_guard::spawn_and_wait(cmd) instead of cmd.status().await,
preserving the existing diagnostic mapping and node spawn error context. Keep
the command configuration and returned status-code behavior unchanged.
In `@crates/aube/src/commands/script_settings.rs`:
- Line 54: Preserve each embedder environment entry’s EnvMerge mode when
constructing extra_env in crates/aube/src/commands/script_settings.rs:54-54. In
crates/aube-scripts/src/lib.rs:695-702, apply Append to the resolved or
inherited environment value and use Replace as an override, while retaining the
existing NODE_OPTIONS behavior.
In `@crates/aube/src/engines.rs`:
- Around line 111-118: Update resolve_node_version and its OnceLock<PROBED>
caching so probe_node_version is not reused across different active runtime
contexts. Key the cached result by the current node_execpath (or
effective_node_version(None)), or invalidate it whenever that runtime context
changes, while preserving the existing override and explicitly cached
runtime-version paths.
In `@crates/aube/src/runtime.rs`:
- Around line 410-444: Update child_env_vars() to accumulate EnvMerge::Append
values per key while iterating ctx.env, using the previously computed value as
the base for subsequent appends instead of re-reading std::env::var_os. Emit one
final entry per key, preserving Replace semantics and ensuring multiple appends
such as NODE_OPTIONS compose in order.
---
Outside diff comments:
In `@crates/aube/src/commands/run.rs`:
- Around line 450-479: The resolved project directory is not passed to
ensure_installed, causing freshness checks to use the process cwd instead of the
requested project tree. Update ensure_installed and every affected caller to
accept and use the resolved directory, including the run.rs flow around
load_manifest and the exec.rs call site at lines 120-127; pass cwd/effective_cwd
rather than recomputing it from crate::dirs::cwd().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e39dcfad-fd48-486a-9915-54c656bcdf6c
📒 Files selected for processing (19)
crates/aube-ffi/src/lib.rscrates/aube-node/src/lib.rscrates/aube-scripts/src/lib.rscrates/aube/src/commands/add/mod.rscrates/aube/src/commands/ci.rscrates/aube/src/commands/deploy/mod.rscrates/aube/src/commands/dlx.rscrates/aube/src/commands/doctor.rscrates/aube/src/commands/exec.rscrates/aube/src/commands/install/args.rscrates/aube/src/commands/install/mod.rscrates/aube/src/commands/node.rscrates/aube/src/commands/run.rscrates/aube/src/commands/runtime.rscrates/aube/src/commands/script_settings.rscrates/aube/src/embed.rscrates/aube/src/engines.rscrates/aube/src/runtime.rsdocs/embedding/rust.md
| .as_ref() | ||
| .and_then(|r| r.node_execpath.clone().or_else(|| r.node_program.clone())) | ||
| .or_else(aube_runtime::node_on_path), | ||
| extra_env: crate::runtime::embedder_extra_env(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve EnvMerge for lifecycle-script environment entries.
embedder_extra_env() discards each entry’s merge mode, and the script layer consequently overwrites every non-NODE_OPTIONS key. Thus EmbedderRuntime::env_append("FOO", "bar") does not append for lifecycle scripts, despite its “every spawn” contract; only the special NODE_OPTIONS path works.
crates/aube/src/commands/script_settings.rs#L54-L54: retainEnvMergealongside each embedder environment entry.crates/aube-scripts/src/lib.rs#L695-L702: applyAppendagainst the resolved/inherited value andReplaceas an override.
📍 Affects 2 files
crates/aube/src/commands/script_settings.rs#L54-L54(this comment)crates/aube-scripts/src/lib.rs#L695-L702
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aube/src/commands/script_settings.rs` at line 54, Preserve each
embedder environment entry’s EnvMerge mode when constructing extra_env in
crates/aube/src/commands/script_settings.rs:54-54. In
crates/aube-scripts/src/lib.rs:695-702, apply Append to the resolved or
inherited environment value and use Replace as an override, while retaining the
existing NODE_OPTIONS behavior.
Two accommodations surfaced by studying nub's production fork (the wrapper embedder from discussion #1083): - EmbedderRuntime::internal_node: splits the node aube's internal machinery spawns (pnpmfile hooks, security scanner, version probes) from the wrapper shim, so a wrapping host can keep aube's hot paths on the real binary while user-facing spawns stay wrapped. Defaults to node_program (no behavior change unless set). - embed::{run,exec,dlx,node} gain an optional per-call EmbedderRuntime, scoped via the task-local runtime slot. set_embedder_runtime is set-once, so hosts that provision a fresh shim dir per command (nub does) need per-call granularity on the non-install paths too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 4 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff297dc. Configure here.
…env merge Review feedback from the bots on #1086: - ensure_installed_in: embed::run/exec resolve the project from an explicit base_dir, but the auto-install freshness check anchored at the process cwd and installed the wrong tree. Thread the resolved dir through and target the auto-install at it. - current(): a process-wide embedder runtime is now authoritative over a RUNTIME an earlier ensure() may have populated, so a late set_embedder_runtime can't leave dlx/exec/run/node on a pre-registration runtime while install scopes see the new one. - engines: version probe is memoized per program path, so a runtime that became active after an earlier bare-node probe gets its own binary probed instead of a stale cached result. - fold_env: embedder env now accumulates — multiple appends to one key chain in order, a later append composes onto a prior replace, and the jailed script path preserves append/replace semantics instead of flattening to replace. - node::run_spawn uses process_guard::spawn_and_wait so an embedded Node child is torn down with the host instead of orphaned. - Drop EnvMerge from the embed re-export (hosts use env_append/env_set); document dlx's process-cwd switch; refresh a stale node_exe comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the review in Fixed:
Deliberately not changed:
This comment was generated by Claude. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
crates/aube/src/engines.rs:139
- This
expect("probe cache poisoned")can panic in embedded/library contexts. Prefer recovering from poisoning by taking the inner guard.
This comment was generated by GitHub Copilot.
.expect("probe cache poisoned")
| match runtime { | ||
| Some(rt) => { | ||
| scope(async move { | ||
| seed_install_embedder_runtime(Some(&rt)); | ||
| future.await | ||
| }) | ||
| .await | ||
| } | ||
| None => future.await, | ||
| } |
| std::sync::Mutex<std::collections::HashMap<std::path::PathBuf, Option<String>>>, | ||
| > = std::sync::OnceLock::new(); | ||
| let cache = PROBED.get_or_init(|| std::sync::Mutex::new(std::collections::HashMap::new())); | ||
| if let Some(cached) = cache.lock().expect("probe cache poisoned").get(program) { |
…isoned probe lock Second review round on #1086: - Per-call embed runtime now rides a dedicated PER_CALL_RUNTIME task-local that survives the fresh scope install::run opens, so a wrapper passed to embed::dlx / embed::run reaches lifecycle scripts in the nested (transient / auto-) install instead of being dropped (cursor, High). - with_embedder_runtime always opens its own runtime scope, so an embed call resolves Node into an isolated slot rather than reusing the process-wide RUNTIME an earlier call populated — sequential calls to different projects no longer cross-contaminate (cursor / copilot). - engines version-probe cache recovers from a poisoned lock (into_inner) instead of expect()-panicking, since it runs in embedded/library contexts (copilot). Earlier-round items (auto-install cwd anchoring, late-registration split, per-path version memo, env-merge accumulation) were already addressed in a93ee82; the re-flags were against pre-fix commits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Second review round addressed in Fixed (still-valid against the latest commit):
Already fixed in
Still standing by design: 674 lib tests + 19 engines tests pass; clippy/fmt clean. This comment was generated by Claude. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/aube/src/runtime.rs:437
with_embedder_runtime(None, ...)opens a freshscope, which makescurrent()consult onlyINSTALL_RUNTIME. Because the slot starts empty and nothing seeds it fromEMBEDDER_CONTEXT, embed entrypoints likeembed::run/exec/dlx/nodewon’t honor a process-wideset_embedder_runtime(...)(they’ll fall back to resolving their own runtime viaensure_for_cwd). Seed the scoped slot from the process-wide embedder runtime (when present) before running the future.
This comment was generated by GitHub Copilot.
None => scope(future).await,
b9ee8bc made with_embedder_runtime always open an isolation scope so sequential embed calls don't reuse each other's resolved runtime — but the `None` arm opened the scope without seeding it. Inside a scope, `ensure` resolves into the empty slot and `current()`'s process-wide fallbacks are never consulted, so a *globally* registered set_embedder_runtime was silently bypassed on embed::{run,exec,dlx,node} — dependencies ran unwrapped, the exact failure mode #1083 exists to prevent. - with_embedder_runtime: both arms now seed the fresh scope through seed_install_embedder_runtime(None) (per-call -> process-wide -> none); the Some arm publishes PER_CALL_RUNTIME and relies on the same chain instead of a duplicated inline set. - ensure: the no-scope branch prefers EMBEDDER_CONTEXT over RUNTIME, so a late registration can't hand callers a stale pre-registration context; the now-redundant (and silently-failing) RUNTIME pre-fill in set_embedder_runtime is gone. - scope_current: also propagates PER_CALL_RUNTIME, since task-locals don't cross tokio::spawn. - Regression test: none_arm_still_seeds_from_the_ambient_embedder_runtime (exercised via the per-call tier — the process-wide global is once-per-process and would leak across unit tests; both tiers flow through the same seed path). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
On a closer pass over The bug. The fix. Both arms of
Test. 675 lib tests pass; clippy/fmt clean. This comment was generated by Claude. |
| if let Some(bin) = &ctx.node_program { | ||
| println!(" bin: {}", bin.display()); | ||
| } |
| if let Some(bin) = &ctx.node_program { | ||
| s.push("node-bin", display_path_owned(bin.clone())); | ||
| } |
|
Looks good! This'll work for Nub perfectly as-is. |

Implements the proposal in discussion #1083: extend the embedder Node hook from #1079 so a host can describe a Node invocation rather than a bin dir — covering wrappers (instrumenting runtimes, transpiling loaders, sandboxes), not just version managers.
What changed
EmbedderRuntimebuilder — two named constructors so no invalid combination is representable:selector(bin_dir)— the version-manager case; reproduces feat(embed): let hosts supply the node runtime for lifecycle scripts #1079'snode_bin_direxactly.wrapper(node_program).real_node().path_dir().version().env_append()/.env_set()— the shim onPATHand atNODE(so$NODE child.jsstays wrapped), the real binary atnpm_node_execpath(sonode-gypfinds Node's install prefix).The three spawn signals —
bin_dir(PATH),node_program(NODE+ spawn program),node_execpath(npm_node_execpath) — are now distinct inRuntimeContext.Lifecycle-script env contribution —
env_append/env_set.NODE_OPTIONSappends over the user'snodeOptionsby default (a wrapper's--importpreload adds rather than clobbers);env_setreplaces. Applied to every spawn: jailed lifecycle scripts (folded throughmerge_node_options) and dlx/exec/node (inherit-and-append).Process-wide registration —
set_embedder_runtime(first-write-wins, mirroringset_embedder) covers every spawn path. A per-callInstallOptions.runtimefully replaces it. Honored regardless ofruntime_switching— an explicit invocation is an override, not a resolution.Lazy version — supplied on the runtime, not probed at seed time;
engines::probe_node_versionprobesnode_execpath(memoized) only when a version is actually needed.Every spawn path —
exec/runare decoupled from process cwd (they take an explicitbase_dir, so concurrent embed calls in different projects don't race), andnodegains a supervised-spawn variant (the CLI path image-replaces the process). New entry pointsembed::{run, exec, dlx, node}drive the run-class commands in process, each rooted at an explicitproject_dir.Every field is optional; unset reproduces standalone aube's behavior byte-for-byte, so
selectoris the degenerate case ({ bin_dir }).Tests
runtime: selector/wrapper derivation,NODE_OPTIONSappend fold, scoped seed + set-once, no-op outside scope.aube-scripts:NODEvsnpm_node_execpathstamped distinctly for a wrapper; execpath falls back to program for a selector;extra_envapplied.cargo test -p aube --lib(670) +-p aube-scripts(76) pass;cargo clippy --all-targetsandcargo fmt --checkclean.Docs: new "Node runtime" and "Run scripts, binaries, and Node" sections in
docs/embedding/rust.md.🤖 Generated with Claude Code
Note
Medium Risk
Touches every Node spawn path (scripts, install lifecycle, pnpmfile, scanners) and changes the embed install API from a bin dir to a richer runtime model; incorrect shim/execpath wiring could break native builds or concurrent multi-project embed calls.
Overview
Replaces the embedder
node_bin_dirhook withEmbedderRuntime:selector(bin_dir)for toolchain pickers (same behavior as before) andwrapper(shim)with optionalreal_node,internal_node,path_dir,version, andenv_append/env_setsoNODEcan stay on a shim whilenpm_node_execpathpoints at the real binary node-gyp expects.set_embedder_runtimeregisters that description process-wide; per-installInstallOptions.runtimeand per-call overrides on newembed::{run, exec, dlx, node}win viawith_embedder_runtimeand aPER_CALL_RUNTIMEtask-local so nested installs (auto-install, dlx scratch) still see the host’s wrapper.Lifecycle and non-jailed spawns get split
NODE/npm_node_execpath, foldedNODE_OPTIONS, andextra_env; aube’s own Node-driven paths useinternal_node_program()to skip wrapper hops.run/exec/dlx/ auto-install anchor on an explicitbase_dir/project_dirinstead of process cwd;aube nodegains a supervisedrun_spawnpath that applies the same child env as scripts.Engines version probing is keyed off
node_execpathwith a per-binary cache. C/Node-API bindings still passruntime: None(ambient node). Docs add embedding sections for runtime modes and in-process run-class commands.Reviewed by Cursor Bugbot for commit e3c6cd2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
NODEvsnpm_node_execpath,NODE_OPTIONSmerging, and embedderextra_envordering).Documentation
Tests
NODE/npm_node_execpathstamping and fallbacks.