From 449dacf02e6681fbb00fc07963e0af1817f30e80 Mon Sep 17 00:00:00 2001 From: Sumit Kumar Date: Sun, 22 Mar 2026 17:20:56 +0530 Subject: [PATCH] fix(link-bins): apply bin ownership overrides in conflict resolution (#10975) BIN_OWNER_OVERRIDES was only used in checkGlobalBinConflicts for global installs. This change applies the same ownership rules in compareCommandsInConflict so that conflict resolution is consistent between global conflict checking and actual bin linking. This ensures packages like npm get priority for bins like npx even in non-global installs. Closes #10850 * test(link-bins): add missing fixture for bin-owner-override test * refactor: extract BIN_OWNER_OVERRIDES to @pnpm/package-bins Move shared logic to avoid code duplication between link-bins and checkGlobalBinConflicts. * fix(link-bins): use regex for Windows path compatibility in test * refactor(link-bins): remove redundant ownName field pkgOwnsBin already handles the binName === pkgName case, making the ownName field and its associated checks redundant. * Change versioning to patch for bins resolver and linker Added BIN_OWNER_OVERRIDES and pkgOwnsBin to @pnpm/bins.resolver for improved conflict resolution in bin linking. * test: remove node_modules from bin-owner-override fixture Move fixture packages to the directory root instead of nesting them inside node_modules, avoiding committing node_modules to the repo. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Zoltan Kochan Co-authored-by: Claude Opus 4.6 (1M context) --- .../apply-bin-owner-overrides-link-bins.md | 9 +++++++ bins/linker/src/index.ts | 11 ++++---- .../fixtures/bin-owner-override/node/index.js | 1 + .../fixtures/bin-owner-override/node/npx.js | 1 + .../bin-owner-override/node/package.json | 8 ++++++ .../bin-owner-override/npm/bin/npm-cli.js | 2 ++ .../bin-owner-override/npm/bin/npx-cli.js | 2 ++ .../bin-owner-override/npm/package.json | 8 ++++++ .../bin-owner-override/other-pkg/index.js | 2 ++ .../bin-owner-override/other-pkg/package.json | 7 +++++ bins/linker/test/index.ts | 27 +++++++++++++++++++ bins/resolver/src/index.ts | 14 ++++++++++ .../commands/src/checkGlobalBinConflicts.ts | 16 +---------- .../fixtures/bin-owner-override/.gitignore | 2 ++ .../node_modules/node/index.js | 1 + .../node_modules/node/npx.js | 1 + .../node_modules/node/package.json | 8 ++++++ .../node_modules/npm/bin/npm-cli.js | 2 ++ .../node_modules/npm/bin/npx-cli.js | 2 ++ .../node_modules/npm/package.json | 8 ++++++ .../node_modules/other-pkg/index.js | 2 ++ .../node_modules/other-pkg/package.json | 7 +++++ 22 files changed, 121 insertions(+), 20 deletions(-) create mode 100644 .changeset/apply-bin-owner-overrides-link-bins.md create mode 100644 bins/linker/test/fixtures/bin-owner-override/node/index.js create mode 100644 bins/linker/test/fixtures/bin-owner-override/node/npx.js create mode 100644 bins/linker/test/fixtures/bin-owner-override/node/package.json create mode 100644 bins/linker/test/fixtures/bin-owner-override/npm/bin/npm-cli.js create mode 100644 bins/linker/test/fixtures/bin-owner-override/npm/bin/npx-cli.js create mode 100644 bins/linker/test/fixtures/bin-owner-override/npm/package.json create mode 100644 bins/linker/test/fixtures/bin-owner-override/other-pkg/index.js create mode 100644 bins/linker/test/fixtures/bin-owner-override/other-pkg/package.json create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/.gitignore create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/index.js create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/npx.js create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/package.json create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npm-cli.js create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npx-cli.js create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/package.json create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/index.js create mode 100644 pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/package.json diff --git a/.changeset/apply-bin-owner-overrides-link-bins.md b/.changeset/apply-bin-owner-overrides-link-bins.md new file mode 100644 index 0000000000..33c38d1731 --- /dev/null +++ b/.changeset/apply-bin-owner-overrides-link-bins.md @@ -0,0 +1,9 @@ +--- +"@pnpm/bins.resolver": patch +"@pnpm/bins.linker": patch +"pnpm": patch +--- + +Added `BIN_OWNER_OVERRIDES` and `pkgOwnsBin` to `@pnpm/bins.resolver`. Applied in bins.linker conflict resolution for consistent behavior between global conflict checking and actual bin linking, so packages like `npm` get priority for bins like `npx` even in non-global installs [#10850](https://github.com/pnpm/pnpm/issues/10850). + +Removed the redundant `ownName` field from `CommandInfo` since `pkgOwnsBin` already handles the `binName === pkgName` case. diff --git a/bins/linker/src/index.ts b/bins/linker/src/index.ts index 783a630f59..eb886603af 100644 --- a/bins/linker/src/index.ts +++ b/bins/linker/src/index.ts @@ -2,7 +2,7 @@ import { existsSync, promises as fs } from 'node:fs' import { createRequire } from 'node:module' import path from 'node:path' -import { type Command, getBinsFromPackageManifest } from '@pnpm/bins.resolver' +import { type Command, getBinsFromPackageManifest, pkgOwnsBin } from '@pnpm/bins.resolver' import { PnpmError } from '@pnpm/error' import { readModulesDir } from '@pnpm/fs.read-modules-dir' import { globalWarn, logger } from '@pnpm/logger' @@ -122,7 +122,6 @@ export async function linkBinsOfPackages ( } interface CommandInfo extends Command { - ownName: boolean pkgName: string pkgVersion: string makePowerShellShim: boolean @@ -169,8 +168,11 @@ function resolveCommandConflicts (group: CommandInfo[], binsDir: string): Comman } function compareCommandsInConflict (a: CommandInfo, b: CommandInfo): number { - if (a.ownName && !b.ownName) return 1 - if (!a.ownName && b.ownName) return -1 + // Check ownership: a package that owns the bin name gets priority + const aOwns = pkgOwnsBin(a.name, a.pkgName) + const bOwns = pkgOwnsBin(b.name, b.pkgName) + if (aOwns && !bOwns) return 1 + if (!aOwns && bOwns) return -1 if (a.pkgName !== b.pkgName) return a.pkgName.localeCompare(b.pkgName) // it's pointless to compare versions of 2 different package return semver.compare(a.pkgVersion, b.pkgVersion) } @@ -235,7 +237,6 @@ async function getPackageBinsFromManifest (manifest: DependencyManifest, pkgDir: } return cmds.map((cmd) => ({ ...cmd, - ownName: cmd.name === manifest.name, pkgName: manifest.name, pkgVersion: manifest.version, makePowerShellShim: POWER_SHELL_IS_SUPPORTED && manifest.name !== 'pnpm', diff --git a/bins/linker/test/fixtures/bin-owner-override/node/index.js b/bins/linker/test/fixtures/bin-owner-override/node/index.js new file mode 100644 index 0000000000..6c32a9cd1b --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/node/index.js @@ -0,0 +1 @@ +console.log('node') diff --git a/bins/linker/test/fixtures/bin-owner-override/node/npx.js b/bins/linker/test/fixtures/bin-owner-override/node/npx.js new file mode 100644 index 0000000000..9fa565183f --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/node/npx.js @@ -0,0 +1 @@ +console.log('npx from node') diff --git a/bins/linker/test/fixtures/bin-owner-override/node/package.json b/bins/linker/test/fixtures/bin-owner-override/node/package.json new file mode 100644 index 0000000000..2097a675b0 --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/node/package.json @@ -0,0 +1,8 @@ +{ + "name": "node", + "version": "1.0.0", + "bin": { + "node": "index.js", + "npx": "npx.js" + } +} diff --git a/bins/linker/test/fixtures/bin-owner-override/npm/bin/npm-cli.js b/bins/linker/test/fixtures/bin-owner-override/npm/bin/npm-cli.js new file mode 100644 index 0000000000..59cb236cde --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/npm/bin/npm-cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('npm cli') diff --git a/bins/linker/test/fixtures/bin-owner-override/npm/bin/npx-cli.js b/bins/linker/test/fixtures/bin-owner-override/npm/bin/npx-cli.js new file mode 100644 index 0000000000..b0a542404f --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/npm/bin/npx-cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('npx cli') diff --git a/bins/linker/test/fixtures/bin-owner-override/npm/package.json b/bins/linker/test/fixtures/bin-owner-override/npm/package.json new file mode 100644 index 0000000000..4fec99d931 --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/npm/package.json @@ -0,0 +1,8 @@ +{ + "name": "npm", + "version": "10.0.0", + "bin": { + "npm": "bin/npm-cli.js", + "npx": "bin/npx-cli.js" + } +} diff --git a/bins/linker/test/fixtures/bin-owner-override/other-pkg/index.js b/bins/linker/test/fixtures/bin-owner-override/other-pkg/index.js new file mode 100644 index 0000000000..65aafa49e8 --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/other-pkg/index.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('other-pkg npx') diff --git a/bins/linker/test/fixtures/bin-owner-override/other-pkg/package.json b/bins/linker/test/fixtures/bin-owner-override/other-pkg/package.json new file mode 100644 index 0000000000..065d2bb7af --- /dev/null +++ b/bins/linker/test/fixtures/bin-owner-override/other-pkg/package.json @@ -0,0 +1,7 @@ +{ + "name": "other-pkg", + "version": "1.0.0", + "bin": { + "npx": "index.js" + } +} diff --git a/bins/linker/test/index.ts b/bins/linker/test/index.ts index 6b1367eef9..d866242940 100644 --- a/bins/linker/test/index.ts +++ b/bins/linker/test/index.ts @@ -697,3 +697,30 @@ describe('node binary linking', () => { expect(fs.existsSync(path.join(binTarget, `node${CMD_EXTENSION}`))).toBe(false) }) }) + +test('linkBins() resolves conflicts using BIN_OWNER_OVERRIDES (npx owned by npm)', async () => { + const binTarget = temporaryDirectory() + const binOwnerOverrideFixture = f.prepare('bin-owner-override') + const warn = jest.fn() + + await linkBins(binOwnerOverrideFixture, binTarget, { warn }) + + // npx should be linked from npm package (owner override), not node or other-pkg + // BIN_OWNER_OVERRIDES says: npx is owned by npm + expect(binsConflictLogger.debug).toHaveBeenCalledWith( + expect.objectContaining({ + binaryName: 'npx', + binsDir: binTarget, + linkedPkgName: 'npm', + skippedPkgName: expect.any(String), + skippedPkgVersion: expect.any(String), + }) + ) + + const binLocation = path.join(binTarget, 'npx') + expect(fs.existsSync(binLocation)).toBe(true) + const content = fs.readFileSync(binLocation, 'utf8') + // npx should come from npm package, not node or other-pkg + // Use a regex that matches both forward and backslashes for Windows compatibility + expect(content).toMatch(/npm[/\\]bin[/\\]npx-cli\.js/) +}) diff --git a/bins/resolver/src/index.ts b/bins/resolver/src/index.ts index 69e15d1848..0b14652b56 100644 --- a/bins/resolver/src/index.ts +++ b/bins/resolver/src/index.ts @@ -9,6 +9,20 @@ export interface Command { path: string } +// Maps a bin name to all packages that are legitimate owners of it, beyond +// the default rule that a package named `X` owns the `X` bin. For example, +// `npx` ships inside the `npm` package, and `pnpx` ships inside both the +// `pnpm` package and the `@pnpm/exe` package. +export const BIN_OWNER_OVERRIDES: Record = { + npx: ['npm'], + pnpm: ['@pnpm/exe'], + pnpx: ['pnpm', '@pnpm/exe'], +} + +export function pkgOwnsBin (binName: string, pkgName: string): boolean { + return binName === pkgName || BIN_OWNER_OVERRIDES[binName]?.includes(pkgName) === true +} + export async function getBinsFromPackageManifest (manifest: DependencyManifest, pkgPath: string): Promise { if (manifest.bin) { return commandsFromBin(manifest.bin, manifest.name, pkgPath) diff --git a/global/commands/src/checkGlobalBinConflicts.ts b/global/commands/src/checkGlobalBinConflicts.ts index 2dbb75c8e5..afcc0e1647 100644 --- a/global/commands/src/checkGlobalBinConflicts.ts +++ b/global/commands/src/checkGlobalBinConflicts.ts @@ -1,7 +1,7 @@ import fs from 'node:fs' import path from 'node:path' -import { getBinsFromPackageManifest } from '@pnpm/bins.resolver' +import { getBinsFromPackageManifest, pkgOwnsBin } from '@pnpm/bins.resolver' import { PnpmError } from '@pnpm/error' import { type GlobalPackageInfo, @@ -10,20 +10,6 @@ import { import { safeReadPackageJsonFromDir } from '@pnpm/pkg-manifest.reader' import type { DependencyManifest } from '@pnpm/types' -// Maps a bin name to all packages that are legitimate owners of it, beyond -// the default rule that a package named `X` owns the `X` bin. For example, -// `npx` ships inside the `npm` package, and `pnpx` ships inside both the -// `pnpm` package and the `@pnpm/exe` package. -const BIN_OWNER_OVERRIDES: Record = { - npx: ['npm'], - pnpm: ['@pnpm/exe'], - pnpx: ['pnpm', '@pnpm/exe'], -} - -function pkgOwnsBin (binName: string, pkgName: string): boolean { - return binName === pkgName || BIN_OWNER_OVERRIDES[binName]?.includes(pkgName) === true -} - /** * Checks for bin name conflicts between new packages and existing global * packages. Returns a set of bin names that should be skipped during linking diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/.gitignore b/pkg-manager/link-bins/test/fixtures/bin-owner-override/.gitignore new file mode 100644 index 0000000000..5867a0493e --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/.gitignore @@ -0,0 +1,2 @@ +!**/node_modules/**/* +!/node_modules/ diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/index.js b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/index.js new file mode 100644 index 0000000000..6c32a9cd1b --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/index.js @@ -0,0 +1 @@ +console.log('node') diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/npx.js b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/npx.js new file mode 100644 index 0000000000..9fa565183f --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/npx.js @@ -0,0 +1 @@ +console.log('npx from node') diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/package.json b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/package.json new file mode 100644 index 0000000000..2097a675b0 --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/node/package.json @@ -0,0 +1,8 @@ +{ + "name": "node", + "version": "1.0.0", + "bin": { + "node": "index.js", + "npx": "npx.js" + } +} diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npm-cli.js b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npm-cli.js new file mode 100644 index 0000000000..59cb236cde --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npm-cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('npm cli') diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npx-cli.js b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npx-cli.js new file mode 100644 index 0000000000..b0a542404f --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/bin/npx-cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('npx cli') diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/package.json b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/package.json new file mode 100644 index 0000000000..4fec99d931 --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/npm/package.json @@ -0,0 +1,8 @@ +{ + "name": "npm", + "version": "10.0.0", + "bin": { + "npm": "bin/npm-cli.js", + "npx": "bin/npx-cli.js" + } +} diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/index.js b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/index.js new file mode 100644 index 0000000000..65aafa49e8 --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/index.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('other-pkg npx') diff --git a/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/package.json b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/package.json new file mode 100644 index 0000000000..065d2bb7af --- /dev/null +++ b/pkg-manager/link-bins/test/fixtures/bin-owner-override/node_modules/other-pkg/package.json @@ -0,0 +1,7 @@ +{ + "name": "other-pkg", + "version": "1.0.0", + "bin": { + "npx": "index.js" + } +}