Repository navigation
fix(embed): keep wrapper node first on PATH - #1188
Conversation
📝 WalkthroughWalkthroughThe change adds runtime-specific PATH precedence for wrapper and selector Node runtimes. Scoped script settings now carry this policy, shared PATH helpers centralize ordering, command paths use the helper, and embedding documentation and tests reflect the resulting behavior. ChangesRuntime PATH precedence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RuntimeResolver
participant RuntimeContext
participant ScriptSettingsState
participant CommandBuilder
RuntimeResolver->>RuntimeContext: resolve bin_dir and precedence
RuntimeContext->>ScriptSettingsState: configure script PATH policy
ScriptSettingsState->>CommandBuilder: provide ordered project and runtime bins
CommandBuilder->>CommandBuilder: append inherited PATH entries
Possibly related PRs
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 SummaryPreserve wrapper-first PATH ordering without adding a required field to the public script-settings structure.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported public struct-literal compatibility issue is resolved because the ordering flag now lives in private state rather than as a required Important Files Changed
Reviews (3): Last reviewed commit: "fix(embed): preserve script settings com..." | Re-trigger Greptile |
4664a79 to
af95d76
Compare
Instruction counts
No instruction-count regression above 1%. Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
af95d76 to
ad90f9e
Compare
## Summary - add `EmbedderRuntime::without_path()` to preserve the inherited `PATH` - continue using the configured runtime for direct aube spawns and `NODE` - make `path_dir(...)` and `without_path()` follow last-call-wins builder semantics - document and test wrapper and selector behavior ## Why Embedding hosts may need to supply the Node program and npm exec path without changing how bare `node` commands resolve. Previously, wrapper runtimes always derived a PATH entry from `node_program.parent()`, so that configuration was not expressible. This is a stacked follow-up to #1188 and targets its branch so this PR contains only the explicit PATH opt-out. It can be retargeted to `main` after #1188 merges. ## Validation - `cargo fmt --check` - `cargo clippy --all-targets -- -D warnings` - `cargo test` *AI-assisted — Tool: Codex; model: openai/gpt-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Additive embedder builder API with unchanged defaults; only affects hosts that opt in via `without_path()`. > > **Overview** > Adds **`EmbedderRuntime::without_path()`** so embedding hosts can keep the inherited **`PATH`** while still setting **`NODE`**, **`npm_node_execpath`**, and using the configured program for direct aube spawns. Wrapper runtimes no longer always prepend the shim’s parent directory to **`PATH`**. > > **`path_dir(...)`** and **`without_path()`** use **last-call-wins** builder semantics ( **`path_dir`** clears the opt-out; **`without_path`** wins if called last). **`selector`** with **`without_path()`** still resolves **`node_program`** from the bin dir but does not prepend it to **`PATH`**. > > Docs and unit tests cover wrapper, selector, and builder ordering. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a36c67a. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Summary
.bindirectories while retaining selector precedencerun, and shell-modeexec/dlxWhy
A wrapper runtime could be bypassed when a dependency or project exposed a
.bin/node, because every runtime bin directory was placed after local package bins. This silently defeated instrumentation, transpilation, or sandboxing supplied by the embedding host.This addresses the gap reported in Discussion #1083.
Validation
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo testAI-assisted — Tool: Codex; model: openai/gpt-5; version: unavailable.
Note
Medium Risk
Changes how PATH is built for all script and shell spawns when a runtime is active; behavior shift is intentional for wrappers but could surprise projects that relied on a local
nodeoverriding the embedder shim.Overview
Fixes wrapper embedder runtimes being silently bypassed when a dependency or project exposed a
node(or other binary) undernode_modules/.bin, which defeated instrumentation/sandbox shims.PATH ordering is now explicit per runtime kind:
EmbedderRuntime::wrappersetsbin_dir_precedes_project_binsso the shim directory is prepended before project-local bins; selectors and resolved version-manager runtimes keep the old rule—.binfirst, runtime bin after. Shared helperorder_path_entries/path_entries_with_project_binsapply the same rules in lifecyclerun_script,aube run, shell-modeexec/dlx, and script settings viaset_script_settings_with_path_order.Docs in
docs/embedding/rust.mddescribe selector vs wrapper PATH behavior; unit tests cover both orderings and scoped script settings propagation.Reviewed by Cursor Bugbot for commit ad90f9e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
node-gypbuilds.Documentation
real_nodebehavior in embedding documentation.