Skip to content

feat(npm): ensure npm is managed by corepack when node.corepack=true and node.npm_shim=false#10196

Merged
jdx merged 1 commit into
jdx:mainfrom
roele:issues/10194
Jun 2, 2026
Merged

feat(npm): ensure npm is managed by corepack when node.corepack=true and node.npm_shim=false#10196
jdx merged 1 commit into
jdx:mainfrom
roele:issues/10194

Conversation

@roele

@roele roele commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Ensures npm is managed by corepack if node.corepack=true and node.npm_shim=false as mentioned in #10194

Summary by CodeRabbit

  • Tests

    • Added end-to-end coverage verifying npm remains node-managed when Corepack is enabled, shows Corepack-managed npm behavior, suppresses interactive download prompts, and that npm --version still runs.
  • New Features

    • Node installation now selectively enables Corepack shims and will enable the npm-specific Corepack shim when appropriate, improving npm/Corepack integration during installs.

Copilot AI review requested due to automatic review settings June 1, 2026 18:46
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a6cca5f2-c13c-40cb-8140-578859eab4c3

📥 Commits

Reviewing files that changed from the base of the PR and between 4461426 and c4fa174.

📒 Files selected for processing (2)
  • e2e/core/test_node_npm_shim_slow
  • src/plugins/core/node.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/plugins/core/node.rs
  • e2e/core/test_node_npm_shim_slow

📝 Walkthrough

Walkthrough

This PR adds a NodePlugin helper to run corepack enable npm in the Node install environment (with download prompts disabled) and conditionally invokes it during Node post-install when corepack is enabled but the mise npm shim is disabled; it also adds an E2E test validating bin/npm is corepack-managed and runnable in that configuration.

Changes

npm Corepack Support

Layer / File(s) Summary
npm corepack enable helper and post-install integration
src/plugins/core/node.rs
New enable_npm_corepack_shim() method invokes corepack enable npm with Node toolchain environment. Post-install logic conditionally calls this when corepack is enabled and node.npm_shim is disabled.
End-to-end test for npm corepack mode
e2e/core/test_node_npm_shim_slow
E2E test validates that bin/npm is not the mise wrapper, remains a symlink resolving into Corepack, and that npm --version runs when MISE_NODE_NPM_SHIM=0, MISE_NODE_COREPACK=1, and COREPACK_ENABLE_DOWNLOAD_PROMPT=0.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I nudged the shim and let Corepack dance,
A symlink twirled, npm kept its stance;
Commands still answered with version cheer,
Tests hopped by to give a clear ear—
Rabbit-approved, the path's secure.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: enabling npm management via corepack when node.corepack=true and node.npm_shim=false, which matches both the code changes in node.rs and the new e2e test.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for using Corepack-managed npm when node.corepack=true and node.npm_shim=false, and extends e2e coverage for this configuration.

Changes:

  • Add a corepack enable npm step during Node tool installation when Corepack is enabled and the npm shim is disabled.
  • Add an e2e scenario asserting that npm resolves to a Corepack-managed shim under that configuration.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/plugins/core/node.rs Runs corepack enable npm during install when Corepack is on and npm_shim is off.
e2e/core/test_node_npm_shim_slow Adds an e2e test case validating the Corepack-managed npm behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/plugins/core/node.rs Outdated
Comment thread src/plugins/core/node.rs Outdated
Comment thread e2e/core/test_node_npm_shim_slow Outdated
Comment thread e2e/core/test_node_npm_shim_slow Outdated
@greptile-apps

greptile-apps Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR ensures that when node.corepack=true and node.npm_shim=false, the npm binary is managed by Corepack (via corepack enable npm) rather than left as a plain Node symlink. A corresponding e2e test verifies the symlink resolves into the corepack directory and that npm remains functional.

  • src/plugins/core/node.rs: Adds enable_npm_corepack_shim (runs corepack enable npm) and calls it from install_version_ when corepack is active and the mise npm shim is disabled.
  • e2e/core/test_node_npm_shim_slow: Adds a third scenario that sets MISE_NODE_COREPACK=1 + MISE_NODE_NPM_SHIM=0, reinstalls node, and asserts the npm symlink resolves under the corepack path.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to the corepack+npm_shim=false combination, and the new e2e test directly validates the expected symlink behavior.

The new function is a straightforward wrapper around corepack enable npm, called only when the user explicitly opts into corepack and opts out of the mise npm shim. The logic is guarded by both conditions, the ordering relative to other install steps is correct, and the e2e test covers the exact scenario introduced.

No files require special attention.

Important Files Changed

Filename Overview
src/plugins/core/node.rs Adds enable_npm_corepack_shim function that runs corepack enable npm selectively when node.corepack=true and node.npm_shim=false, correctly placed after the existing enable_default_corepack_shims call.
e2e/core/test_node_npm_shim_slow Adds a third test case verifying that when MISE_NODE_COREPACK=1 and MISE_NODE_NPM_SHIM=0, the npm binary symlink resolves into the corepack directory and remains functional.

Reviews (3): Last reviewed commit: "feat(npm): ensure npm is managed by core..." | Re-trigger Greptile

Comment thread e2e/core/test_node_npm_shim_slow Outdated
@jdx jdx merged commit 423ebcd into jdx:main Jun 2, 2026
33 checks passed
@roele roele deleted the issues/10194 branch June 3, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants