Repository navigation
fix: handle npm link <package-name> in npm shim (auto-reshim) - #11748
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changesnpm link reshim handling
Estimated code review effort: 2 (Simple) | ~10 minutes 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 SummaryThe PR expands npm-link classification to distinguish workspace-local links from global links and attempts to preserve npm’s exit status when reshimming.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix: distinguish global vs workspace-loc..." | 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: 1
🤖 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 `@src/plugins/core/assets/node_npm_shim`:
- Around line 60-61: Update the should_reshim logic around additional_bare_cmds
so npm link commands targeting a workspace are excluded from the global reshim
path. Explicitly recognize --workspace=<workspace> and other workspace-local
link forms, while retaining global-link detection for --workspace without a
target and existing package-name links.
🪄 Autofix
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 15555d29-df3d-4d80-be71-e191cdaa3b48
📒 Files selected for processing (1)
src/plugins/core/assets/node_npm_shim
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 `@src/plugins/core/assets/node_npm_shim`:
- Around line 21-33: The npm link argument parser must distinguish global
workspace links from workspace-local links. Update the parsing logic around
has_workspace_flag to track whether a package operand is present, consume
workspace values for --workspace=<name>, --workspace <name>, and -w <name>, and
suppress reshim only when both a workspace option and package operand indicate a
workspace-local link; allow npm link --workspace <name> to run mise reshim.
- Around line 86-92: Update the npm wrapper flow around the command invocation
and reshim call so both statuses are captured safely under set -euo pipefail
using if/else or || lists. Preserve npm’s status in npm_exit, allow mise reshim
to run regardless of npm failure, and return npm_exit unconditionally after
reshim completes.
🪄 Autofix
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 22f63bae-76bc-49a1-824d-f90b118c79c8
📒 Files selected for processing (1)
src/plugins/core/assets/node_npm_shim
| # Run npm and capture its exit code | ||
| "$node_bin" "$npm_cli" "$@" | ||
| local npm_exit=$? | ||
| printf "Reshimming mise %s...\n" "$plugin_name" >&2 | ||
| mise reshim | ||
| # Return npm's exit code, not mise reshim's | ||
| return $npm_exit |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
nl -ba src/plugins/core/assets/node_npm_shim | sed -n '1,105p'Repository: jdx/mise
Length of output: 186
🏁 Script executed:
#!/usr/bin/env bash
set -u
path="src/plugins/core/assets/node_npm_shim"
if [ -f "$path" ]; then
awk '{printf "%6d\t%s\n", NR, $0}' "$path" | sed -n '1,105p'
else
echo "missing: $path"
fi
printf '\n--- relevant occurrences ---\n'
awk '/# Run npm and capture its exit code|mise reshim|return \$npm_exit/ {printf "%6d\t%s\n", NR, $0}' "$path"Repository: jdx/mise
Length of output: 3542
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
script="$tmpdir/breakpoint.sh"
cat > "$script" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail
run_failing_command_can_use_exit=$?
local npm_exit=$?
printf "npm_exit is empty: %s\n" "$npm_exit"
printf "run_failing_command_can_use_exit is: %s\n" "$run_failing_command_can_use_exit"
npm_cmd="$1"
"$npm_cmd" || exit $npm_cmd
local npm_exit=$?
printf "npm_exit captured: %s\n" "$npm_exit"
reshim_cmd="$2"
"$reshim_cmd"
local reshim_exit=$?
printf "npm_exit is: %s\n" "$npm_exit"
printf "reshim_exit is: %s\n" "$reshim_exit"
return "$npm_exit"
EOF
chmod +x "$script"
bash "$tmpdir/breakpoint.sh" "shopt -u nocasematch false" "shopt -u nocasematch true"
printf '"%s"\n' "$?"Repository: jdx/mise
Length of output: 227
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
script="$tmpdir/breakpoint.sh"
cat > "$script" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail
run_failing_command_can_use_exit=$?
local npm_exit=$?
printf 'run_failing_command_can_use_exit is: %s\n' "$run_failing_command_can_use_exit"
echo "line 6 reached"
npm_cmd="$1"
"$npm_cmd" || exit $npm_cmd
local npm_exit=$?
printf 'npm_exit captured: %s\n' "$npm_exit"
reshim_cmd="$2"
"$reshim_cmd" || exit $?
local reshim_exit=$?
printf 'npm_exit is: %s\n' "$npm_exit"
printf 'reshim_exit is: %s\n' "$reshim_exit"
return "$npm_exit"
EOF
chmod +x "$script"
bash "$tmpdir/breakpoint.sh" "false" "true"
printf 'final status: "%s"\n' "$?"Repository: jdx/mise
Length of output: 227
🏁 Script executed:
#!/usr/bin/env bash
set -u
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
script="$tmpdir/breakpoint.sh"
cat > "$script" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail
run_failing_command_can_use_exit=$?
local npm_exit=$?
printf 'run_failing_command_can_use_exit is: %s\n' "$run_failing_command_can_use_exit"
echo "line 6 reached"
npm_cmd="$1"
"$npm_cmd" || exit $npm_cmd
local npm_exit=$?
printf 'npm_exit captured: %s\n' "$npm_exit"
reshim_cmd="$2"
"$reshim_cmd" || exit $?
local reshim_exit=$?
printf 'npm_exit is: %s\n' "$npm_exit"
printf 'reshim_exit is: %s\n' "$reshim_exit"
return "$npm_exit"
EOF
chmod +x "$script"
(set +e; bash "$tmpdir/breakpoint.sh" "false" "true"); printf 'final status: "%s"\n' "$?"Repository: jdx/mise
Length of output: 245
Preserve npm’s exit status under set -e.
With set -euo pipefail, a failed npm command exits at Line 87 before npm_exit is assigned, and a failed mise reshim exits before return $npm_exit. Capture each command’s status in if/else or || lists, then return npm_exit unconditionally.
🤖 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 `@src/plugins/core/assets/node_npm_shim` around lines 86 - 92, Update the npm
wrapper flow around the command invocation and reshim call so both statuses are
captured safely under set -euo pipefail using if/else or || lists. Preserve
npm’s status in npm_exit, allow mise reshim to run regardless of npm failure,
and return npm_exit unconditionally after reshim completes.
…t code under set -e
Summary
The npm shim (
src/plugins/core/assets/node_npm_shim) already handles:npm link(links current directory globally) ✅npm link ./pathor../path(links local directory) ✅But misses
npm link <package-name>(links from registry), which also installs globally by creating a symlink in{prefix}/node_modules→{prefix}/lib/node_modules/<package>.Changes
File:
src/plugins/core/assets/node_npm_shim(lines 51-65)Changed the
if/ifchain toif/elif/elifto properly handle all three cases:Testing
Why This Fix Is Correct
npm link <pkg>IS a global install — it creates a symlink in the globalnode_modules, same asnpm install -glink, just had a logic gapmise install node@<version>mise reshimhas file locking (PR fix(shim): prevent recursion when directory env is filtered #10982)Related
install_npm_shim()innode.rs:434-439Ready to merge — minimal, focused, fixes the actual bug without architectural changes.
Summary by CodeRabbit
npm linkhandling for workspace flags and local package links..or/; bare and package-name links retain global behavior.mise reshimencounters an error.