From 9517da53ac7dff5ac4f1f8d3cffd764c62002789 Mon Sep 17 00:00:00 2001 From: sunjm13 Date: Fri, 14 Aug 2026 20:55:28 +0545 Subject: [PATCH] fix(arborist): prefer direct dependency bins --- workspaces/arborist/lib/arborist/rebuild.js | 11 +- workspaces/arborist/test/arborist/rebuild.js | 103 +++++++++++++++++++ 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/workspaces/arborist/lib/arborist/rebuild.js b/workspaces/arborist/lib/arborist/rebuild.js index 53c6f63e77d94..4f6ad2cc8a7e5 100644 --- a/workspaces/arborist/lib/arborist/rebuild.js +++ b/workspaces/arborist/lib/arborist/rebuild.js @@ -15,6 +15,8 @@ const { resolve, delimiter } = require('node:path') const { isScriptAllowed } = require('../script-allowed.js') const boolEnv = b => b ? '1' : '' +const isDirectDependency = node => [...node.edgesIn] + .some(edge => edge.from.isProjectRoot || edge.from.isWorkspace) const sortNodes = (a, b) => (a.depth - b.depth) || localeCompare(a.path, b.path) const _checkBins = Symbol.for('checkBins') @@ -391,9 +393,12 @@ module.exports = cls => class Builder extends cls { const timeEnd = time.start('build:link') const promises = [] - // sort the queue by node path, so that the module-local collision - // detector in bin-links will always resolve the same way. - for (const node of queue.sort(sortNodes)) { + const directDependencies = new Set(queue.filter(isDirectDependency)) + // Direct dependencies must claim colliding bins before transitive ones. + // Depth and path retain a deterministic fallback within each group. + queue.sort((a, b) => + (directDependencies.has(b) - directDependencies.has(a)) || sortNodes(a, b)) + for (const node of queue) { // TODO these run before they're awaited promises.push(this.#createBinLinks(node)) } diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index 6c3062be89a00..f0b55c8c2aee3 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -20,6 +20,10 @@ new MockRegistry({ const isWindows = process.platform === 'win32' +const binTarget = (path, name) => isWindows + ? fs.readFileSync(resolve(path, 'node_modules/.bin', `${name}.cmd`), 'utf8') + : fs.readlinkSync(resolve(path, 'node_modules/.bin', name)) + // Most rebuild tests pre-date the allowScripts gate and assert that // install scripts run. Bypass the default-deny in this suite by // default; individual tests that want to assert the gate's behaviour @@ -60,6 +64,105 @@ t.test('rebuild bin links only for specified node', async t => { t.throws(() => fs.statSync(mkdirp), 'mkdirp bin not linked') }) +t.test('direct dependencies win root bin collisions', async t => { + const pkg = (name, bin = 'shared') => ({ + 'package.json': JSON.stringify({ + name, + version: '1.0.0', + bin: { shared: bin }, + }), + [bin]: '#!/usr/bin/env node\n', + }) + + t.test('direct aliases take precedence over hoisted transitive dependencies', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'direct-alias-root', + dependencies: { + parent: '1.0.0', + 'z-direct': 'npm:direct-provider@1.0.0', + }, + }), + node_modules: { + 'a-transitive': pkg('a-transitive'), + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + dependencies: { 'a-transitive': '1.0.0' }, + }), + }, + 'z-direct': pkg('direct-provider'), + }, + }) + + await newArb({ path }).rebuild() + t.match(binTarget(path, 'shared'), /z-direct/, 'links the direct alias bin') + }) + + t.test('workspace-root direct dependencies take precedence', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'workspace-root', + workspaces: ['packages/*'], + }), + node_modules: { + 'a-transitive': pkg('a-transitive'), + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + dependencies: { 'a-transitive': '1.0.0' }, + }), + }, + workspace: t.fixture('symlink', '../packages/workspace'), + 'z-direct': pkg('z-direct'), + }, + packages: { + workspace: { + 'package.json': JSON.stringify({ + name: 'workspace', + version: '1.0.0', + dependencies: { + parent: '1.0.0', + 'z-direct': '1.0.0', + }, + }), + }, + }, + }) + + await newArb({ path }).rebuild() + t.match(binTarget(path, 'shared'), /z-direct/, 'links the workspace direct bin') + }) + + t.test('equally transitive dependencies retain deterministic path ordering', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'transitive-root', + dependencies: { parent: '1.0.0' }, + }), + node_modules: { + 'a-provider': pkg('a-provider'), + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + dependencies: { + 'a-provider': '1.0.0', + 'z-provider': '1.0.0', + }, + }), + }, + 'z-provider': pkg('z-provider'), + }, + }) + + await newArb({ path }).rebuild() + t.match(binTarget(path, 'shared'), /a-provider/, 'uses path order as a stable fallback') + }) +}) + t.test('rebuild no matching nodes', async t => { const path = fixture(t, 'dep-installed-without-bin-link') const semver = resolve(path, 'node_modules/.bin/semver')