fix(cli): resolve bundled skills by proximity, not candidate order

.changeset/staged-skill-resolution-proximity.md

@@ -0,0 +1,5 @@
+---
+"hunkdiff": patch
+---
+
+Resolve bundled skills from the nearest matching directory, preferring `hunkdiff/skills`, then `skills`, then `node_modules/hunkdiff/skills` within that directory to avoid unrelated ancestor and nested-package copies.

packages/hunk/src/core/run/paths.test.ts

@@ -96,6 +96,94 @@ describe("paths", () => {
     }
   });
 
+  test("prefers a staged skill beside the binary over a generic one further up", () => {
+    const tempRoot = createTempRoot("hunk-skill-proximity-");
+
+    try {
+      // A source install stages its skills under `hunkdiff/` beside the executable, so a
+      // reviewer with their own `skills/` directory anywhere above the bin directory must
+      // not shadow it. Exhausting the generic shape to the filesystem root first did.
+      const installDir = join(tempRoot, ".local", "bin");
+      const installedSkill = join(installDir, "hunkdiff", "skills", "hunk-review", "SKILL.md");
+      const unrelatedSkill = join(tempRoot, "skills", "hunk-review", "SKILL.md");
+      const fakeBinary = join(installDir, "hunk");
+
+      mkdirSync(dirname(installedSkill), { recursive: true });
+      mkdirSync(dirname(unrelatedSkill), { recursive: true });
+      writeFileSync(installedSkill, "# installed\n");
+      writeFileSync(unrelatedSkill, "# unrelated\n");
+      writeFileSync(fakeBinary, "binary\n");
+
+      expect(resolveBundledSkillPath("hunk-review", [fakeBinary])).toBe(installedSkill);
+    } finally {
+      rmSync(tempRoot, { recursive: true, force: true });
+    }
+  });
+
+  test("prefers Hunk's staging tree over generic skills and a nested package", () => {
+    const tempRoot = createTempRoot("hunk-skill-specificity-");
+
+    try {
+      // All three shapes at one ancestor: the source install's namespaced copy wins.
+      const installedSkill = join(tempRoot, "hunkdiff", "skills", "hunk-review", "SKILL.md");
+      const staleGenericSkill = join(tempRoot, "skills", "hunk-review", "SKILL.md");
+      const staleNestedSkill = join(
+        tempRoot,
+        "node_modules",
+        "hunkdiff",
+        "skills",
+        "hunk-review",
+        "SKILL.md",
+      );
+      const fakeBinary = join(tempRoot, "hunk");
+
+      for (const skill of [installedSkill, staleGenericSkill, staleNestedSkill]) {
+        mkdirSync(dirname(skill), { recursive: true });
+      }
+      writeFileSync(installedSkill, "# installed\n");
+      writeFileSync(staleGenericSkill, "# stale generic\n");
+      writeFileSync(staleNestedSkill, "# stale nested\n");
+      writeFileSync(fakeBinary, "binary\n");
+
+      expect(resolveBundledSkillPath("hunk-review", [fakeBinary])).toBe(installedSkill);
+    } finally {
+      rmSync(tempRoot, { recursive: true, force: true });
+    }
+  });
+
+  test("prefers a prebuilt artifact's own skills over a stale nested package", () => {
+    const tempRoot = createTempRoot("hunk-skill-prebuilt-");
+
+    try {
+      // A prebuilt release artifact ships `skills/` beside the binary with no `hunkdiff/`
+      // wrapper (see `stagePrebuiltArtifact`), so the shape that protects it from a stale
+      // `node_modules/hunkdiff` is `skills` ranking above `node_modules/hunkdiff/skills`.
+      // Deliberately omits `hunkdiff/` so only that pair decides the result: the
+      // source-install case above passes either way and cannot pin this ordering.
+      const shippedSkill = join(tempRoot, "skills", "hunk-review", "SKILL.md");
+      const staleNestedSkill = join(
+        tempRoot,
+        "node_modules",
+        "hunkdiff",
+        "skills",
+        "hunk-review",
+        "SKILL.md",
+      );
+      const fakeBinary = join(tempRoot, "hunk");
+
+      for (const skill of [shippedSkill, staleNestedSkill]) {
+        mkdirSync(dirname(skill), { recursive: true });
+      }
+      writeFileSync(shippedSkill, "# shipped\n");
+      writeFileSync(staleNestedSkill, "# stale\n");
+      writeFileSync(fakeBinary, "binary\n");
+
+      expect(resolveBundledSkillPath("hunk-review", [fakeBinary])).toBe(shippedSkill);
+    } finally {
+      rmSync(tempRoot, { recursive: true, force: true });
+    }
+  });
+
   test("canonicalizes two spellings of one directory to the same path", () => {
     // Canonicalize with the same resolver the code under test uses: plain
     // realpathSync leaves Windows 8.3 short names (RUNNER~1) in place, which

packages/hunk/src/core/run/paths.ts

@@ -118,8 +118,14 @@ export function resolveInstalledExtensionsRoot(env: NodeJS.ProcessEnv = process.
   return extensionsDir ? join(extensionsDir, INSTALLED_EXTENSIONS_DIR_NAME) : undefined;
 }
 
-/** Search one path and its parents for one relative child path. */
-function findRelativePathFromAncestors(startPath: string, relativePath: string) {
+/**
+ * Search one path and its parents for the first of several relative child paths.
+ *
+ * Proximity wins over candidate order: every shape is tested at one ancestor before the
+ * walk moves up. Exhausting one shape to the filesystem root first would let a generic
+ * match far above the start path beat the specific match sitting right at it.
+ */
+function findRelativePathFromAncestors(startPath: string, relativePaths: readonly string[]) {
   let current = resolve(startPath);
 
   try {
@@ -131,9 +137,11 @@ function findRelativePathFromAncestors(startPath: string, relativePath: string)
   }
 
   for (;;) {
-    const candidate = join(current, relativePath);
-    if (fs.existsSync(candidate)) {
-      return candidate;
+    for (const relativePath of relativePaths) {
+      const candidate = join(current, relativePath);
+      if (fs.existsSync(candidate)) {
+        return candidate;
+      }
     }
 
     const parent = dirname(current);
@@ -148,27 +156,28 @@ function findRelativePathFromAncestors(startPath: string, relativePath: string)
 /**
  * Resolve one bundled skill's path from source, npm, or prebuilt package layouts.
  *
- * Every shipped skill lives at `skills/<name>/SKILL.md` in all three layouts, so
- * the name is the only thing that varies and the search itself stays one walk.
+ * Every shipped skill lives at `skills/<name>/SKILL.md` in all three layouts, so the name is
+ * the only thing that varies and the search stays one walk. Within each directory, prefer
+ * Hunk's namespaced staging tree, then standalone skills, then a nested npm package.
  */
 export function resolveBundledSkillPath(
   name: BundledSkillName = DEFAULT_BUNDLED_SKILL_NAME,
   searchRoots?: string[],
 ) {
   const roots = searchRoots ?? [import.meta.dir, process.execPath];
   const skillRelativePath = join("skills", name, "SKILL.md");
+  // Prefer the Hunk-specific staging tree over generic skills. Both shipped layouts outrank
+  // node_modules/hunkdiff, which may belong to another project and contain a stale copy.
   const relativeCandidates = [
-    skillRelativePath,
     join("hunkdiff", skillRelativePath),
+    skillRelativePath,
     join("node_modules", "hunkdiff", skillRelativePath),
   ];
 
   for (const root of roots) {
-    for (const relativePath of relativeCandidates) {
-      const resolvedPath = findRelativePathFromAncestors(root, relativePath);
-      if (resolvedPath) {
-        return resolvedPath;
-      }
+    const resolvedPath = findRelativePathFromAncestors(root, relativeCandidates);
+    if (resolvedPath) {
+      return resolvedPath;
     }
   }