diff --git a/.changeset/staged-skill-resolution-proximity.md b/.changeset/staged-skill-resolution-proximity.md new file mode 100644 index 000000000..7abe287f1 --- /dev/null +++ b/.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. diff --git a/packages/hunk/src/core/run/paths.test.ts b/packages/hunk/src/core/run/paths.test.ts index 0c24bc347..7ff74b478 100644 --- a/packages/hunk/src/core/run/paths.test.ts +++ b/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 diff --git a/packages/hunk/src/core/run/paths.ts b/packages/hunk/src/core/run/paths.ts index 33f61f850..ec9f65f12 100644 --- a/packages/hunk/src/core/run/paths.ts +++ b/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,8 +156,9 @@ 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//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//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, @@ -157,18 +166,18 @@ export function resolveBundledSkillPath( ) { 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; } }