Repository navigation
feat(scripts): match pnpm's full npm_* env for lifecycle & run scripts - #879
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements pnpm-compatible npm_package_* environment variable export across manifest parsing, runtime node discovery, script settings propagation, node-gyp shim wiring, and lifecycle command execution and tests. Changespnpm environment variable parity and script lifecycle wiring
Sequence DiagramsequenceDiagram
participant CLI as Aube CLI
participant Settings as ScriptSettings
participant Manifest as PackageJson
participant Runner as spawn_shell Process
CLI->>Settings: configure_script_settings(command)
CLI->>Manifest: read package.json
CLI->>Settings: apply_script_settings_env(cmd)
Settings->>Runner: set npm_execpath/npm_node_execpath/NODE/npm_command/npm_config_node_gyp
Manifest->>Runner: provide npm_package_env() pairs
CLI->>Runner: spawn_shell(shell_cmd) with npm_lifecycle_script & npm_package_json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Greptile SummaryThis PR adds full
Confidence Score: 5/5Safe to merge — the env-var plumbing is well-tested and the one rough edge (unhandled bootstrap exception in the JS shim) only affects error-path UX. The core logic — scrub-then-stamp, BTreeMap-ordered engines, jail/non-jail path handling, lazy shim writes — is correct and backed by unit tests and bats integration tests. The only finding is a missing try-catch in the node-gyp.js shim that degrades the error message when bootstrap fails; it does not affect the happy path or data correctness. crates/aube/src/commands/install/node_gyp_bootstrap.rs — the node-gyp.js shim's bootstrap error path. Important Files Changed
Reviews (2): Last reviewed commit: "fix: apply ai review suggestions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-runtime/src/discover.rs`:
- Around line 208-210: node_on_path() currently returns find_on_path(...)
directly which can yield relative or non-executable paths; update node_on_path
to canonicalize the returned PathBuf and verify it is an absolute file and
executable before returning. Specifically: call find_on_path(node_exe_name()),
if Some(p) then attempt std::fs::canonicalize(&p) (handle errors by returning
None), verify the canonical path metadata is a file and that it is executable
(on Unix check permission bits, on Windows use an appropriate fallback such as
checking extensions/exists), and only return Some(canonical_path) when those
checks pass; otherwise return None so npm_node_execpath / NODE can't be set to a
non-absolute or non-runnable path.
In `@crates/aube-scripts/src/lib.rs`:
- Around line 564-575: The helper apply_npm_manifest_env currently overwrites
only allowlisted keys but leaves inherited npm_package_* vars intact; update
apply_npm_manifest_env to first remove/clear any existing environment entries
whose names start with "npm_package_" before calling cmd.env(...) and before
iterating manifest.npm_package_env(); likewise, in the other site where
manifest.npm_package_env() is applied (the manifest.npm_package_env() loop in
crates/aube/src/commands/run.rs), add the same cleanup step to remove existing
npm_package_* entries on the Command/environment before stamping the allowlisted
pairs so no disallowed inherited npm_package_* variables remain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e159ec23-04a9-48a6-b029-93e58e6cfea6
📒 Files selected for processing (15)
crates/aube-manifest/src/lib.rscrates/aube-runtime/src/discover.rscrates/aube-runtime/src/lib.rscrates/aube-scripts/src/lib.rscrates/aube/src/commands/install/mod.rscrates/aube/src/commands/install/node_gyp_bootstrap.rscrates/aube/src/commands/pack.rscrates/aube/src/commands/rebuild.rscrates/aube/src/commands/run.rscrates/aube/src/commands/script_settings.rscrates/aube/src/runtime.rstest/lifecycle_scripts.batstest/node_gyp_bootstrap.batstest/node_gyp_rebuild.batstest/run.bats
There was a problem hiding this comment.
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)
923-930:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't make
aube runfail hard oncurrent_exe()lookup.
apply_script_settings_envincrates/aube-scripts/src/lib.rstreats the same lookup as best-effort, but this path turns it into a hard error. That means acurrent_exe()failure now aborts everyaube run, even when the script never touchesnode-gyp. StampAUBE_NODE_GYP_EXEconditionally instead of bubbling the error.Suggested fix
let mut command = aube_scripts::spawn_shell(&shell_cmd); - command - .env("PATH", &new_path) - .current_dir(cwd) - .env( - "AUBE_NODE_GYP_EXE", - std::env::current_exe().into_diagnostic()?, - ) - .env("AUBE_NODE_GYP_PROJECT_DIR", node_gyp_project_dir) + command.env("PATH", &new_path).current_dir(cwd); + if let Ok(aube_exe) = std::env::current_exe() { + command.env("AUBE_NODE_GYP_EXE", aube_exe); + } + command + .env("AUBE_NODE_GYP_PROJECT_DIR", node_gyp_project_dir) .env("npm_lifecycle_event", script) .env("npm_command", "run-script") .stderr(aube_scripts::child_stderr());🤖 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 923 - 930, The code currently unconditionally calls std::env::current_exe().into_diagnostic()? while setting the AUBE_NODE_GYP_EXE env on the Command in run.rs, so a current_exe() failure will abort aube run; change this to a best-effort lookup and only set the env when the lookup succeeds: call std::env::current_exe().ok() (or handle the Result without returning an error), convert the PathBuf to a string/os-str as needed, and call command.env("AUBE_NODE_GYP_EXE", exe_path) only when Some(path) is present; leave AUBE_NODE_GYP_PROJECT_DIR unchanged. This mirrors apply_script_settings_env’s best-effort behavior and prevents current_exe() failures from bubbling out as hard errors.
🤖 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.
Outside diff comments:
In `@crates/aube/src/commands/run.rs`:
- Around line 923-930: The code currently unconditionally calls
std::env::current_exe().into_diagnostic()? while setting the AUBE_NODE_GYP_EXE
env on the Command in run.rs, so a current_exe() failure will abort aube run;
change this to a best-effort lookup and only set the env when the lookup
succeeds: call std::env::current_exe().ok() (or handle the Result without
returning an error), convert the PathBuf to a string/os-str as needed, and call
command.env("AUBE_NODE_GYP_EXE", exe_path) only when Some(path) is present;
leave AUBE_NODE_GYP_PROJECT_DIR unchanged. This mirrors
apply_script_settings_env’s best-effort behavior and prevents current_exe()
failures from bubbling out as hard errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 670a955d-fd1e-4913-9b73-81cca2c3bdde
📒 Files selected for processing (3)
crates/aube-runtime/src/discover.rscrates/aube-scripts/src/lib.rscrates/aube/src/commands/run.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/aube-runtime/src/discover.rs
Merged
This was referenced Jun 13, 2026
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.
Summary
Lifecycle and
aube runscripts now receive the samenpm_*environment pnpm/npm export, so PM-detection bridges and build tooling behave identically under aube. Previously aube omittednpm_execpath,npm_node_execpath,npm_package_json,npm_command,npm_config_node_gyp,npm_package_engines_*, andnpm_lifecycle_script.Adds
PackageJson::npm_package_env()— pnpm's exact allowlist (name,version, deep-flattenedengines/config/bin), envified the npm way (non-[A-Za-z0-9_]→_, case preserved). Stringbinflattens to the unsuffixednpm_package_bin, matching pnpm (not npm's unscoped-name rule).npm_commandis stamped per command family (run-script,install,rebuild,pack); re-asserted at the run spawn site so a preceding auto-install doesn't clobber it.npm_config_node_gyppoints at a lazy, dependency-free node-gyp stand-in written into aube's cache. It bootstraps the real node-gyp only when a tool actually executes it — no eager install — and self-heals on each write.npm_node_execpath/NODEfall back to the ambientnodeonPATHwhen no runtime switch is active, so they're always populated.Implementation notes
Manifest-derived vars are applied last in
run_script, after the build jail'senv_clear, so jailed dependency builds see them too.Dedups the
node.exe/nodeselection inaube-runtime/discover.rsbehind asingle
node_exe_name()and adds a process-freenode_on_path()PATH lookup.Test plan
cargo clippy --workspace --all-targets -- -D warningscleancargo fmt --checkcleanaube-manifest,aube-scripts,aube-runtimerun,lifecycle_scripts(new pnpm-parity tests),node_gyp_bootstrap,node_gyp_rebuildnpm_package_*set against pnpm 11.5 — byte-for-byte identicalSummary by CodeRabbit
New Features
Tests