ReviewOS

also looking at this

pantry-pm/pantry

fix(install): discover executable nested npm programs without leaking candidates

#270
Closed glennmichael123 wants to merge fix/269-npm-discovery into main
1 file +117 -19
6 hours ago

Refs #269.

The reported defect

discoverNpmBinaries decided executability from statFile(full_path).mode & 0o111. io_helper.statFile sets .mode = if (@hasField(@TypeOf(stat), "mode")) stat.mode else 0, and the pinned Zig 0.17 toolchain's stat type has no mode field — so the mask was always 0 & 0o111, every candidate took the non-executable branch, and that branch never released its full_path allocation. Each nested npm program was both skipped and leaked. The bin//sbin/ loop immediately below has had an else true fallback for exactly this reason; the npm loop never got one.

Reproduced before any change, through public discoverBinaries with a real nested lib/node_modules/example-cli/bin holding one executable and one non-executable file:

expected 1, found 0
[SafeAllocator] (err): leaked [len: 57]
[SafeAllocator] (err): leaked [len: 71]

Both candidate paths — matching the two allocator leaks the issue records.

The fix routes detection through the existing io_helper.isExecutable predicate (libc access(X_OK) on posix, extension check on Windows), frees rejected candidates, and puts the duped name under its own errdefer so a failing append cannot leak it.

A latent precedence bug the fix exposed

discoverBinaries consulted the npm layout first and returned it exclusively — harmless only while that scan always came back empty. With the scan working, that behaviour regresses real packages. packages/ts-pantry/src/recipes/npmjs.com.ts installs to /lib/node_modules/npm and keeps its own /bin, where it deliberately replaces npx with a shim ("our shim fixes a bug where npx doesn't work if ~/.local/lib doesn't exist"). The upstream npm tarball's nested bin/ holds seven executables:

-rwxr-xr-x npm          -rwxr-xr-x npm-cli.js
-rwxr-xr-x npx          -rwxr-xr-x npm-prefix.js
-rwxr-xr-x npm.cmd      -rwxr-xr-x npx-cli.js
-rwxr-xr-x npx.cmd

So with precedence intact, installing npmjs.com would activate commands literally named npm-cli.js, npm-prefix.js and npx.cmd, and would resolve npx to upstream's instead of the shim, because bin/ is never scanned at all. Confirmed by execution, not by reading: a temporary probe restoring exclusive precedence makes the new shadow regression report expected 2, found 3.

The npm layout is now consulted second, only when the package ships no bin/ or sbin/ programs of its own. A package that has both keeps exactly today's behaviour; a package that has only the nested layout now gets its programs.

Verification

  • zig build test: 658 pass, 6 skip, 2 fail — the two failures are the pre-existing shell.install_route bash/zsh cases, which fail identically on main on this machine.
  • No allocator leaks.
  • Both new regressions are filesystem tests over real executable and non-executable fixtures under std.testing.allocator, so a reintroduced leak fails the suite rather than passing silently.
  • The existing activation coverage this touches — exact-version reactivation, different-provider collision and removal, nested launcher resolution, argv0 resources and relocation — all still pass.

Scope

Deliberately Refs, not Closes. The issue also asks for the repair to be independently reviewed and verified through the normal source/release flow before closing, and it is a declared blocker for #268, whose same-provider cross-version activation guard is untouched here and belongs in its own commit.

io_helper.statFile's mode-0 behaviour is left alone: it is relied on elsewhere, and the issue is explicit that no mode value should be fabricated. The bin/sbin loop's own else true fallback is also left alone — switching it to isExecutable would narrow what gets activated on Windows, which is a separate behaviour change.

🤖 Generated with Claude Code

1 changed file on the files tab, with 0 review threads.

Sign in to comment on this pull request.