setup: correct the mechanism in #6, and widen it past npm's own cache

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");
 }
 
 /**

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 });
+  }
 });