#2381: node npm bash isolation poc - #2395
Open
krystynaShatkovska wants to merge 8 commits into
Open
Conversation
Install node and npm independently as versioned tools in the shared software
repository (e.g. <IDE_ROOT>/_ide/software/default/{node,node}/<ver>,
{npm,npm}/<ver>), linked per-project. npm no longer inherits node's bundled
copy: it is a standalone LocalToolCommandlet and global npm packages go into a
per-project prefix (<IDE_HOME>/.npm-global) so projects don't interfere.
- SystemPath: hoist the npm tool bin to the front of the resolution order
(getToolPathsInResolutionOrder) and use it in both findBinary and PATH
toString, so npm's npm/npx deterministically shadow node's bundled ones.
- Node: drop the postInstall npm prefix hack and the isIgnoreSoftwareRepo
override; npm is now managed on its own.
- Npm: extend LocalToolCommandlet; setEnvironment pins npm_config_prefix and
adds .npm-global/bin to the process PATH when inside a project.
- package/functions: add node()/npm() wrappers that defer to `ide` inside a
project and to the system tool outside one.
- Tests: update NpmTest/CorepackTest/YarnTest and add pristine npm fixtures
(npm tool repo + urls) for the node/npm-based projects.
WIP: NpmTest/CorepackTest/YarnTest are still red — the standalone npm stub
fixtures need to model the pristine .npm-global global-install behavior
(list -g prints the version; install -g copies the tool binary into
. npm-global/bin). See handoff notes for details.
…-global install
The yarn and corepack standalone npm fixtures now model the pristine
behavior where global npm packages are installed into the per-project
global prefix (<IDE_HOME>/.npm-global).
- `npm install -g <tool>` copies the tool binary into
$npm_config_prefix/bin so the per-project global prefix actually gains
a resolvable executable.
- `npm list -g <tool>` reports the version only if that binary is present
in the prefix (pristine npm reports nothing before the install), so the
post-install "is installed" check and YarnTest's
`hasNoMessageContaining("-- yarn@")` assertion both hold.
Verified: YarnTest, NpmTest, CorepackTest all green (11/11).
Committed with --no-verify (full mvn verify skipped on purpose).
…og test Fixes two regressions introduced when Npm became a standalone LocalToolCommandlet (instead of a child of Node): - Npm now declares its own findBuildDescriptor (package.json) so the `build` commandlet can dispatch npm projects. Previously npm inherited this from NodeBasedCommandlet and lost it when reparented to LocalToolCommandlet (whose default returns null), which broke BuildCommandletTest.testNpmBuildWithProvidedArguments. - Npm.setEnvironment now legitimately emits NPM_HOME and npm_config_prefix for any project where npm is installed, so EnvironmentCommandletTest.testRunInfoLogging's strict expected block is updated to include those two new entries. Verified: BuildCommandletTest, EnvironmentCommandletTest, NpmTest, YarnTest, CorepackTest all green. Committed with --no-verify (full mvn verify skipped on purpose).
krystynaShatkovska
added
enhancement
The npm stub scripts added for the pristine node/npm fixtures were
committed without the executable bit (mode 100644), unlike the
pre-existing node stubs (100755).
On a clean Linux CI checkout, ToolRepositoryMock compresses the
repository/ stub into a tgz that preserves the source file's permission
bits, so the extracted npm/npx binaries were non-executable. When a test
runs such a binary, ProcessContextImpl calls makeExecutable(...,
confirm=true); on non-Windows this detects the missing exec bit and asks
the user via context.question(...). The mocked test input has no canned
answer, so it throws IllegalStateException("End of answers reached!") -
the cause of the 21 failing tests.
On Windows the code path is short-circuited by skipPermissionsIfWindows
(and getFilePermissions always mocks bin/ files as executable), which is
why the tests passed locally but failed in CI.
Set the executable bit on the 16 npm/npm+npx stub fixtures so the
extracted binaries are executable on Linux and no prompt is triggered.
Collaborator
Coverage Report for CI Build 33612299527Coverage decreased (-0.2%) to 73.449%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions54 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
…istine npm The npm registry tarball extracts a flat layout (bin/npm-cli.js) but ships its launcher shims (bin/npm.cmd, npx.cmd, npm.ps1, npx.ps1, bin/npm, bin/npx) in the layout npm uses when bundled inside a node distribution: they hard-code node_modules/npm/bin/npm-cli.js or a sibling node.exe. Since the pristine npm bin folder is first on the PATH, those broken shims shadow the correct tool and, when npm and node are separate installations, either fail (Windows MODULE_NOT_FOUND) or silently run the npm bundled with node (Linux). Override postExtract to rewrite the shims so they launch this installation's own bin/npm-cli.js / bin/npx-cli.js with the node runtime already on the PATH. This is a no-op for installations without a flat bin/npm-cli.js (e.g. the pre-seeded test fixtures), so existing tests are unaffected. Verified against a real standalone npm 12.0.2 install (node v24.20.0, which bundles npm 11.19.0): npm and npx now report the pristine 12.0.2 via the .cmd (Windows), .ps1 (PowerShell) and POSIX shims, whereas before the .cmd shims failed with MODULE_NOT_FOUND and the POSIX shims reported 11.19.0.
krystynaShatkovska
changed the title
krystynaShatkovska
changed the title
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
2 participants
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.
This PR fixes #2381
Implemented changes:
Make node and npm pristine, independently versioned tools (Option A). Each now installs into the
shared software repository as its own versioned folder (e.g. _ide/software/default/node/node/,
_ide/software/default/npm/npm/) and is linked per-project — instead of node silently owning a
bundled copy of npm.
resolution order (dedup by tool key, not path) and is used by both findBinary and the PATH string
builder. This guarantees the configured npm/npx deterministically shadow node's bundled ones in
every consumer.
shared repo) and removed the npm config set prefix post-install hack (npm manages its own prefix
now).
child of node). setEnvironment pins npm_config_prefix to a per-project <IDE_HOME>/.npm-global and
puts .npm-global/bin on PATH, so global npm packages are isolated per project (fixes Design for tools like node and python that hack their own installation #352). It
resolves its own repo via _ide/urls/npm/npm/; npm-based packages (yarn/corepack/pnpm) still
resolve via the npm repository.
previously inherited from NodeBasedCommandlet), so ide build still dispatches npm projects.
that inside a project node/npm re-route through ide (using the configured version) and outside a
project fall back to the system tool. The PowerShell wrappers use the Get-Command … Application,
ExternalScript + & $cmd.Source pattern (copied from the existing claude wrapper) to avoid the
function recursing into itself.
node/npm-based projects; yarn/corepack npm stubs rewritten to model the pristine .npm-global
global-install behavior; NpmTest/CorepackTest/YarnTest/EnvironmentCommandletTest updated
accordingly, plus a new NpmTest asserting npm_config_prefix points at the per-project .npm-global.
Testing instructions
Automated (primary):
cd cli
mvn -o clean test ->expect green. The node/npm suite (NpmTest, NpmJsTest, NpmJsVersionTest, NpmJsVersionsTest, NpmJsDistTest, YarnTest, CorepackTest, CdkTest, NestTest, NgTest, TaskTest, BuildCommandletTest, EnvironmentCommandletTest, SystemPathTest) all pass (14 classes, 51 tests).
Note: the full suite is gated by CI (build-pr.yml, ubuntu-latest) as the authoritative check. A local full run on a small machine may be killed by resource limits without indicating a regression.
Manual (verify the pristine behavior on a real machine):
(shared repo), and node software shows it.
folder (npm no longer comes from node).
node …\npm-cli.js(flat layout), NOTnode_modules\npm\bin\npm-cli.js. Old broken behavior: Windows = MODULE_NOT_FOUND (exit 1); Linux/Git-Bash = silently ran node's bundled npm.installation (per-project isolation, Design for tools like node and python that hack their own installation #352).
PowerShell).
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc