diff --git a/src/setup.js b/src/setup.js index 859dd18..906033d 100644 --- a/src/setup.js +++ b/src/setup.js @@ -11,22 +11,36 @@ const here = path.dirname(fileURLToPath(import.meta.url)); * `npm link` puts `human-review` on PATH; otherwise fall back to npx, which only * resolves once the package is published. * - * The probe has to discount its own npx run. `npx -y human-review setup --global` - * puts this package on PATH for the duration of that one command, out of npm's - * `_npx` cache, so a naive `which` succeeds and setup writes a bare - * `human-review` into SKILL.md. That binary is gone the moment npx exits, and - * every later agent invocation dies with "command not found". + * The probe has to discount its own runner. `npx -y human-review setup --global` + * puts this package on PATH for the length of that one command, and running + * setup inside a project that depends on human-review puts that project's + * `node_modules/.bin` there too. Either way a naive `which` succeeds and setup + * writes a bare `human-review` into a SKILL.md that is meant to work from any + * directory. The entry drops off PATH as soon as the runner exits, or stops + * resolving as soon as the agent runs from somewhere else. */ export function invocation() { const probe = process.platform === "win32" ? "where" : "which"; const found = spawnSync(probe, ["human-review"], { encoding: "utf8" }); const resolved = found.status === 0 ? found.stdout.trim().split(/\r?\n/)[0].trim() : ""; - return resolved && !isNpxCachePath(resolved) ? "human-review" : "npx -y human-review"; + return resolved && !isTransientBin(resolved) ? "human-review" : "npx -y human-review"; } -/** True for a binary npm placed in its transient `_npx` cache for one command. */ -export function isNpxCachePath(binPath) { - return binPath.split(/[\\/]/).includes("_npx"); +/** + * True for a bin that exists only for one command or only inside one project. + * Every transient channel routes through a `node_modules` directory: `npx`, + * `pnpm dlx`, `yarn dlx`, `bunx`, and a plain project-local dependency. npm's + * npx cache additionally sits under `_npx`, which is worth keeping as its own + * check because npm may relocate the bin inside it. Durable installs resolve + * through a bin directory with neither segment: `npm i -g`, `npm link`, volta, + * nvm, asdf, and the pnpm and yarn globals. + * + * A durable path that happens to contain either segment degrades to the npx + * form, which always works, so the failure direction here is cosmetic. + */ +export function isTransientBin(binPath) { + const segments = binPath.split(/[\\/]/); + return segments.includes("_npx") || segments.includes("node_modules"); } /** diff --git a/test/setup.test.js b/test/setup.test.js index 35b5d84..3ad20d3 100644 --- a/test/setup.test.js +++ b/test/setup.test.js @@ -4,7 +4,7 @@ import os from "node:os"; import path from "node:path"; import test from "node:test"; -import { installSkills, isNpxCachePath } from "../src/setup.js"; +import { installSkills, invocation, isTransientBin } from "../src/setup.js"; test("global setup installs the skill for Claude Code, Codex, and shared agents", () => { const home = fs.mkdtempSync(path.join(os.tmpdir(), "human-review-setup-")); @@ -24,24 +24,58 @@ test("global setup installs the skill for Claude Code, Codex, and shared agents" } }); -test("a binary from npm's _npx cache does not count as installed on PATH", () => { - // `npx -y human-review setup --global` resolves `which human-review` to the - // transient cache copy, which disappears when npx exits. Writing a bare - // `human-review` into SKILL.md on the strength of that leaves every later - // agent run failing with "command not found". +test("a bin that exists only for one command or one project is not an install", () => { + // npx: the package is on PATH for the length of the setup command only. assert.equal( - isNpxCachePath("/Users/x/.npm/_npx/f043fcd613c7efad/node_modules/.bin/human-review"), + isTransientBin("/Users/x/.npm/_npx/f043fcd613c7efad/node_modules/.bin/human-review"), true, ); assert.equal( - isNpxCachePath("C:\\Users\\x\\AppData\\Local\\npm-cache\\_npx\\a1b2\\human-review.cmd"), + isTransientBin( + "C:\\Users\\x\\AppData\\Local\\npm-cache\\_npx\\a1b2\\node_modules\\.bin\\human-review.cmd", + ), true, ); - // A real global install or `npm link` must still win. - assert.equal(isNpxCachePath("/opt/homebrew/bin/human-review"), false); - assert.equal(isNpxCachePath("/usr/local/bin/human-review"), false); + // pnpm dlx, yarn dlx, bunx, and a plain project-local dependency: durable on + // disk, but only resolvable from inside one project. + assert.equal(isTransientBin("/Users/x/project/node_modules/.bin/human-review"), true); + assert.equal(isTransientBin("/Users/x/.cache/bun/node_modules/.bin/human-review"), true); - // `_npx` only counts as a path segment, never as a substring of one. - assert.equal(isNpxCachePath("/Users/x/my_npx_tools/bin/human-review"), false); + // Durable installs must still win. + assert.equal(isTransientBin("/opt/homebrew/bin/human-review"), false); + assert.equal(isTransientBin("/usr/local/bin/human-review"), false); + assert.equal(isTransientBin("/Users/x/.volta/bin/human-review"), false); + + // Both markers count only as whole path segments. + assert.equal(isTransientBin("/Users/x/my_npx_tools/bin/human-review"), false); + assert.equal(isTransientBin("/Users/x/node_modules_backup/bin/human-review"), false); +}); + +test("invocation prefers npx when the only bin on PATH is transient", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "human-review-path-")); + const transient = path.join(root, "_npx", "deadbeef", "node_modules", ".bin"); + const durable = path.join(root, "durable-bin"); + const previous = process.env.PATH; + + try { + for (const dir of [transient, durable]) { + fs.mkdirSync(dir, { recursive: true }); + const bin = path.join(dir, "human-review"); + fs.writeFileSync(bin, "#!/bin/sh\n"); + fs.chmodSync(bin, 0o755); + } + + process.env.PATH = `${transient}${path.delimiter}${previous}`; + assert.equal(invocation(), "npx -y human-review"); + + process.env.PATH = `${durable}${path.delimiter}${previous}`; + assert.equal(invocation(), "human-review"); + + process.env.PATH = `${path.join(root, "empty")}${path.delimiter}${previous}`; + assert.equal(invocation(), "npx -y human-review"); + } finally { + process.env.PATH = previous; + fs.rmSync(root, { recursive: true, force: true }); + } });