Repository navigation
feat(embed): let hosts supply the node runtime for lifecycle scripts - #1079
Conversation
Adds `node_bin_dir` to the embed install/add options. When set, the install seeds its scoped runtime slot (runtime::seed_embedder_node) with that node before `ensure` runs, so lifecycle scripts spawn on the host's node and find it on PATH — instead of falling back to an ambient `node` lookup. Threads through AddToProjectOptions and install::InstallOptions; no-op and behavior-preserving when unset. This lets an embedder (mise) that manages its own node as a dependency run `allow_builds` lifecycle scripts without requiring node on the process PATH — restoring what the subprocess `aube add --global` path provided via prepend_path. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds an optional embedder-provided Node binary directory to install options, propagates it through embedded add/install flows, and seeds the scoped runtime for lifecycle scripts. Standard CLI, CI, and deploy paths default the option to ChangesEmbedder Node Runtime
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Embedder
participant InstallOptions
participant run_inner
participant RuntimeScope
Embedder->>InstallOptions: provide embedder_node_bin_dir
InstallOptions->>run_inner: forward Node binary directory
run_inner->>RuntimeScope: seed_embedder_node
RuntimeScope-->>run_inner: lifecycle scripts use seeded node
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c4edac8. Configure here.
Greptile SummaryThis PR lets embedding hosts provide the Node.js runtime used by lifecycle scripts. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix(embed): make seed_embedder_node asyn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds an embedder-provided Node.js runtime override so embedded aube installs can run dependency lifecycle scripts using a host-managed node (instead of relying on an ambient PATH lookup), by seeding a per-install task-local runtime context before runtime resolution occurs.
Changes:
- Adds
node_bin_dir: Option<PathBuf>to the embed API install options and threads it into install wiring asembedder_node_bin_dir. - Introduces
runtime::seed_embedder_node()to populate the install-scoped runtime slot soruntime::ensure()short-circuits and script spawning uses the host’s Node. - Updates internal install/add entry points and constructors to carry the new option, and adds unit tests for the seeding behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/aube/src/runtime.rs | Adds RuntimeSource::Embedder, implements seed_embedder_node, and adds unit tests around seeding behavior. |
| crates/aube/src/embed.rs | Exposes node_bin_dir on the embed install API and forwards it into internal install options. |
| crates/aube/src/commands/install/mod.rs | Seeds the embedder node dir into the runtime slot before calling runtime::ensure. |
| crates/aube/src/commands/install/args.rs | Extends internal InstallOptions with embedder_node_bin_dir and initializes defaults. |
| crates/aube/src/commands/deploy/mod.rs | Updates InstallOptions construction to include the new field. |
| crates/aube/src/commands/ci.rs | Updates InstallOptions construction to include the new field. |
| crates/aube/src/commands/add/mod.rs | Adds the embedder-facing option on AddToProjectOptions and forwards it into install options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Directory containing the `node` executable an embedding host wants | ||
| /// lifecycle scripts to run on (see | ||
| /// [`crate::commands::install::InstallOptions::embedder_node_bin_dir`]). | ||
| /// `None` keeps aube's own runtime resolution. | ||
| pub embedder_node_bin_dir: Option<std::path::PathBuf>, |
There was a problem hiding this comment.
Fixed in 1352d26 — the public AddToProjectOptions field is now node_bin_dir, matching embed::InstallOptions. The internal install::InstallOptions.embedder_node_bin_dir keeps its name (it's the plumbing target, not embedder-facing).
This reply was generated by an AI coding assistant.
| #[tokio::test] | ||
| async fn seed_embedder_node_is_a_noop_outside_scope() { | ||
| // No install scope active → nothing to seed, and no panic. | ||
| seed_embedder_node(PathBuf::from("/opt/mise/node/bin")); | ||
| assert!(current().is_none()); | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in 1352d26. The test now asserts inside a fresh scope (reads the empty task-local slot) instead of calling current() bare (which falls back to the process-wide RUNTIME), so it's no longer order-dependent.
This reply was generated by an AI coding assistant.
- fix aube-node / aube-ffi AddToProjectOptions literals that omitted the new field (workspace bindings failed to compile) - name the public option node_bin_dir on AddToProjectOptions too, matching embed::InstallOptions (was embedder_node_bin_dir) - absolutize the supplied bin dir so scripts run with a different cwd still resolve node + the prepended PATH entry - probe the supplied node for its version so engine checks and virtual-store hashing key off the same node as lifecycle scripts - make the outside-scope test assert via a fresh scope (task-local), not the process-wide RUNTIME cell, to avoid order-dependence Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
| pub fn seed_embedder_node(bin_dir: PathBuf) { | ||
| // Absolutize: lifecycle scripts may run with a different working | ||
| // directory, so both `node_program()` and the prepended PATH entry must | ||
| // resolve independently of cwd. `absolute` doesn't require the dir to | ||
| // exist or touch symlinks; fall back to the input if it errors. | ||
| let bin_dir = std::path::absolute(&bin_dir).unwrap_or(bin_dir); | ||
| let node_exe = if cfg!(windows) { "node.exe" } else { "node" }; | ||
| let node_bin = bin_dir.join(node_exe); |
There was a problem hiding this comment.
Fixed in df3e68e. seed_embedder_node now checks the runtime slot first and returns before any absolute/version-probe work when it's outside a scope or the slot is already seeded.
This reply was generated by an AI coding assistant.
| let bin_dir = PathBuf::from("/opt/mise/node/bin"); | ||
| seed_embedder_node(bin_dir.clone()); | ||
|
|
||
| let ctx = current().expect("slot seeded"); | ||
| assert_eq!(ctx.source, RuntimeSource::Embedder); | ||
| assert_eq!(ctx.bin_dir.as_deref(), Some(bin_dir.as_path())); | ||
|
|
||
| let node_exe = if cfg!(windows) { "node.exe" } else { "node" }; | ||
| assert_eq!(node_program(), bin_dir.join(node_exe)); | ||
| assert_eq!(path_entries(), vec![bin_dir.clone()]); |
There was a problem hiding this comment.
Already handled in 7b95d66 (before this comment's commit): the test computes its expected via std::path::absolute too, so it matches the seed's own normalization on Windows. windows-latest is green.
This reply was generated by an AI coding assistant.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
crates/aube/src/runtime.rs:149
seed_embedder_nodedocuments that it is a no-op outside an installscope()and when the slot is already populated, but it currently still does work (including anode --versionspawn) before discovering it can't/shouldn't set the task-local slot.
Also, if the supplied bin_dir doesn't actually contain node, the function still seeds a context pointing at a non-existent binary, which causes later spawns to fail and prevents runtime::ensure() from doing normal resolution.
This comment was generated by GitHub Copilot.
// Absolutize: lifecycle scripts may run with a different working
// directory, so both `node_program()` and the prepended PATH entry must
// resolve independently of cwd. `absolute` doesn't require the dir to
// exist or touch symlinks; fall back to the input if it errors.
let bin_dir = std::path::absolute(&bin_dir).unwrap_or(bin_dir);
| options.dangerously_allow_all_builds, | ||
| ); | ||
| install_options.control = options.control; | ||
| install_options.embedder_node_bin_dir = options.node_bin_dir; |
There was a problem hiding this comment.
This compiles — a partial move is fine here. node_bin_dir is the last use of that field, and the subsequent options.offline (a Copy bool) reads a different, still-initialized field. options.control is moved on the line just above for the same reason. Confirmed by green cargo build --workspace / clippy CI.
This reply was generated by an AI coding assistant.
| if let Some(bin_dir) = &opts.embedder_node_bin_dir { | ||
| crate::runtime::seed_embedder_node(bin_dir.clone()); | ||
| } |
There was a problem hiding this comment.
Fixed in df3e68e. seed_embedder_node is now async and probes node --version via tokio::process::Command, so the install task no longer blocks a Tokio worker on the child spawn. The call site in install::run awaits it.
This reply was generated by an AI coding assistant.
- check the runtime slot up front and return before any path/version probing when outside a scope or already seeded - probe node --version via tokio::process so the install task doesn't block a Tokio worker on the child spawn Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
| let ctx = RuntimeContext { | ||
| node_bin: Some(node_bin), | ||
| bin_dir: Some(bin_dir), | ||
| version, | ||
| requested: None, | ||
| source: RuntimeSource::Embedder, | ||
| provenance: RuntimeProvenance::Mise, | ||
| fresh_pin: None, | ||
| }; |
Summary
Adds a way for a host embedding aube (e.g. mise) to supply the node runtime that dependency lifecycle scripts run on, instead of aube falling back to an ambient
nodePATH lookup.Motivation: mise embeds
aube::embed::addto installnpm:tools (jdx/mise#11149) with node managed as a mise dependency — not on the processPATH. Todayruntime::node_program()falls back toPathBuf::from("node")when no runtime switch is active, soallow_buildsinstalls whose dependencies run lifecycle scripts fail to find node. The old subprocess path (aube add --global) avoided this byprepend_path-ing the dependency toolset; this restores that for the in-process path.Change
embed::InstallOptionsandAddToProjectOptionsgainnode_bin_dir: Option<PathBuf>(the dir containing the host'snode). Threads intoinstall::InstallOptions::embedder_node_bin_dir.install::run— within the runtimescope, beforeruntime::ensure—runtime::seed_embedder_node(bin_dir)sets the scoped runtime slot to aRuntimeContextpointing at the host's node (source: Embedder,provenance: Mise).ensurereturns early when the slot is populated, so aube does no runtime resolution of its own andnode_program()/path_entries()return the host's node.tokio::task_local, scoped per-install — no process-global mutation.None): normal runtime switching / PATH fallback.Tests
runtime::tests::seed_embedder_node_drives_node_program_and_path— within ascope, the seed drivesnode_program()andpath_entries(), and is idempotent (a second seed doesn't clobber, matchingensure's early return).seed_embedder_node_is_a_noop_outside_scope— no scope → no-op, no panic.cargo test -p aube --lib(667) passes; clippy clean.This PR was generated by an AI coding assistant (Claude Code).
Note
Medium Risk
Changes how lifecycle scripts resolve Node for embedders that opt in; scoped per-install via task-local state, but incorrect
node_bin_dircould break builds or version checks for those hosts.Overview
Adds
node_bin_diron embed APIs (embed::InstallOptions,AddToProjectOptions) andembedder_node_bin_dironinstall::InstallOptions, wired throughembed::install/add_to_project.Before
runtime::ensure, install callsruntime::seed_embedder_node, which fills the per-install task-local runtime slot with the host’snode(RuntimeSource::Embedder), prepends that bin dir to script PATH, and probes--versionso engine checks match the same binary.ensurethen skips its own resolution when the slot is already set.CLI, CI, deploy, FFI, and Node-API callers keep
None(unchanged behavior). New unit tests cover seeding inside an install scope and no-op behavior outside it.Reviewed by Cursor Bugbot for commit df3e68e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
nodeexecutable directory.noderesolution.Bug Fixes
Compatibility
nodedirectory is provided across CLI, CI, deployment, and FFI paths.Tests