Skip to content

fix(run): prepend node_modules/.bin to script PATH unconditionally#283

Merged
colinhacks merged 1 commit into
mainfrom
bin-path-unconditional
Jul 3, 2026
Merged

fix(run): prepend node_modules/.bin to script PATH unconditionally#283
colinhacks merged 1 commit into
mainfrom
bin-path-unconditional

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

bin_path() gated each ancestor's node_modules/.bin PATH segment on an is_dir() check computed before spawn, so a .bin absent at spawn was dropped — including one a script creates mid-run (install-then-invoke). npm and pnpm prepend unconditionally without stat'ing; a missing PATH entry is inert. Drop the check; keep the 16-level cap and workspace_root break. find_bin() untouched.

Tests: bin_path_includes_missing_dir (unit) and run_script_finds_bin_created_mid_run (Unix e2e).

Closes #281

https://claude.ai/code/session_01Xuh9u8b93eMNu53fTQTWX7

bin_path() gated each ancestor's node_modules/.bin PATH segment on an
is_dir() check computed once before the child spawns, so a .bin absent at
spawn was dropped from the child's PATH entirely. A script that creates
node_modules/.bin/<tool> mid-run and then invokes <tool> by bare name (the
install-then-invoke pattern) could not find it. npm and pnpm both prepend
node_modules/.bin unconditionally, without stat'ing; a missing PATH entry is
harmless. Drop the existence check; keep the ancestor-walk cap and the
workspace_root break.

Closes #281
Copilot AI review requested due to automatic review settings July 2, 2026 15:37
@vercel

vercel Bot commented Jul 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview, Comment Jul 2, 2026 3:39pm

Request Review

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes — drops the pre-spawn is_dir() gate in bin_path so each ancestor node_modules/.bin is prepended to a script's PATH unconditionally, fixing install-then-invoke scripts (#281) that create .bin mid-run.

  • Unconditional .bin prepend in bin_path — removes the is_dir() check; the walk-up now pushes every node_modules/.bin entry, preserving the 16-level cap and the workspace_root break. Matches npm/pnpm, which prepend without stat'ing; non-existent PATH entries are inert.
  • Unit test bin_path_includes_missing_dir — asserts a non-existent <root>/node_modules/.bin still appears in the returned PATH.
  • Unix e2e run_script_finds_bin_created_mid_run — a script that writes+chmods node_modules/.bin/mytool and then invokes it by bare name must succeed.

Verified both consumers of bin_path (cli.rs:3499, cli.rs:4864) merely concatenate the result into the child PATH, so the extra non-existent entries are harmless. find_bin is untouched. Clean, well-scoped fix that faithfully matches the referenced npm/pnpm behavior.

ℹ️ Nitpicks

  • crates/nub-core/src/workspace/scripts.rs — the for _ in 0..16 loop now pushes on iteration 0 before any break, so dirs is never empty and the if dirs.is_empty() branch (~L322) is now dead. Harmless; can be simplified to always take the join path in a follow-up.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes nub run script PATH construction so node_modules/.bin entries are prepended even if the directory doesn’t exist at spawn time, matching npm/pnpm behavior and fixing the install-then-invoke pattern from #281.

Changes:

  • Remove the is_dir() gating in bin_path() so ancestor node_modules/.bin segments are always prepended (still capped at 16 levels; still breaks at workspace_root).
  • Add a unit regression test ensuring bin_path() includes a non-existent .bin directory.
  • Add a Unix-only e2e regression test proving a script can create node_modules/.bin/<tool> mid-run and then invoke it by bare name successfully.

Reviewed changes

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

File Description
crates/nub-core/src/workspace/scripts.rs Make bin_path() prepend node_modules/.bin unconditionally; add unit regression test for missing .bin inclusion.
crates/nub-cli/tests/workspace_run.rs Add Unix e2e regression test covering mid-run creation of .bin and subsequent bare-name invocation.

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

@colinhacks colinhacks merged commit 8a47969 into main Jul 3, 2026
49 checks passed
@colinhacks

Copy link
Copy Markdown
Contributor Author

Shipped in v0.3.0: https://github.com/nubjs/nub/releases/tag/v0.3.0

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.

nub run: node_modules/.bin missing from script PATH when the directory does not exist at spawn

2 participants