From 10cd9345c5110c13a07b072cc95b1a5ac945090a Mon Sep 17 00:00:00 2001 From: shashank-100 Date: Sun, 6 Sep 2026 10:58:02 +0530 Subject: [PATCH 1/4] fix(cli): resolve bundled skills by proximity, not candidate order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveBundledSkillPath` walked each candidate shape to the filesystem root before trying the next one, so the generic `skills//SKILL.md` shape was tested at every ancestor before `hunkdiff/skills//SKILL.md` was tried at the install directory — which is exactly where `scripts/install-bin.ts` stages skills for a source install. A user with an unrelated `~/skills/hunk-review/SKILL.md` therefore got that file instead of the installed one. The `hunkdiff/` nesting exists to avoid claiming generic skill names beside the executable; the resolver's loop order defeated that intent. Test every candidate shape at one ancestor before moving up, and order the candidates most-specific-first so a directory holding several layouts resolves to the one that names Hunk explicitly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019jAgP7j1gaT9fSk5syLHDH --- .../staged-skill-resolution-proximity.md | 5 ++ packages/hunk/src/core/run/paths.test.ts | 53 +++++++++++++++++++ packages/hunk/src/core/run/paths.ts | 33 +++++++----- 3 files changed, 79 insertions(+), 12 deletions(-) create mode 100644 .changeset/staged-skill-resolution-proximity.md diff --git a/.changeset/staged-skill-resolution-proximity.md b/.changeset/staged-skill-resolution-proximity.md new file mode 100644 index 000000000..802f55d77 --- /dev/null +++ b/.changeset/staged-skill-resolution-proximity.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Resolve bundled skills from the closest matching layout instead of the first shape found while walking to the filesystem root. A source install stages its skills under `hunkdiff/` beside the executable, so an unrelated `skills/` directory anywhere above the bin directory no longer shadows the installed skill. diff --git a/packages/hunk/src/core/run/paths.test.ts b/packages/hunk/src/core/run/paths.test.ts index 0c24bc347..80537c5ae 100644 --- a/packages/hunk/src/core/run/paths.test.ts +++ b/packages/hunk/src/core/run/paths.test.ts @@ -96,6 +96,59 @@ 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 the packaged layout over a generic skills directory beside it", () => { + const tempRoot = createTempRoot("hunk-skill-specificity-"); + + try { + // Both shapes at one ancestor: the one naming Hunk explicitly is the bundled skill, + // and the bare `skills/` sibling belongs to whatever else lives in that directory. + const packagedSkill = join( + tempRoot, + "node_modules", + "hunkdiff", + "skills", + "hunk-review", + "SKILL.md", + ); + const siblingSkill = join(tempRoot, "skills", "hunk-review", "SKILL.md"); + const fakeBinary = join(tempRoot, "hunk"); + + mkdirSync(dirname(packagedSkill), { recursive: true }); + mkdirSync(dirname(siblingSkill), { recursive: true }); + writeFileSync(packagedSkill, "# packaged\n"); + writeFileSync(siblingSkill, "# sibling\n"); + writeFileSync(fakeBinary, "binary\n"); + + expect(resolveBundledSkillPath("hunk-review", [fakeBinary])).toBe(packagedSkill); + } 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..c97b61e12 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); @@ -157,18 +165,19 @@ export function resolveBundledSkillPath( ) { const roots = searchRoots ?? [import.meta.dir, process.execPath]; const skillRelativePath = join("skills", name, "SKILL.md"); + // Most specific first, so a directory holding several layouts resolves to the one that + // names Hunk explicitly. A source install stages its skills under `hunkdiff/` beside the + // executable precisely to avoid claiming the generic `skills/` name in a bin directory. const relativeCandidates = [ - skillRelativePath, - join("hunkdiff", skillRelativePath), join("node_modules", "hunkdiff", skillRelativePath), + join("hunkdiff", skillRelativePath), + 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; } } From 9efa6353db38ff2335d92fad7a81b616d8554c0f Mon Sep 17 00:00:00 2001 From: shashank-100 Date: Sun, 6 Sep 2026 11:31:08 +0530 Subject: [PATCH 2/4] docs(changeset): keep the summary to one user-facing sentence CONTRIBUTING requires non-empty Changeset summaries to be a single user-facing sentence; the original had two and the second described the implementation rather than the user-visible effect. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019jAgP7j1gaT9fSk5syLHDH --- .changeset/staged-skill-resolution-proximity.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/staged-skill-resolution-proximity.md b/.changeset/staged-skill-resolution-proximity.md index 802f55d77..9cbf80561 100644 --- a/.changeset/staged-skill-resolution-proximity.md +++ b/.changeset/staged-skill-resolution-proximity.md @@ -2,4 +2,4 @@ "hunkdiff": patch --- -Resolve bundled skills from the closest matching layout instead of the first shape found while walking to the filesystem root. A source install stages its skills under `hunkdiff/` beside the executable, so an unrelated `skills/` directory anywhere above the bin directory no longer shadows the installed skill. +Stop an unrelated `skills/` directory above the install location from shadowing the bundled skill a source install staged beside the Hunk executable. From 1bff5896ab63e4fed8723f79f79ca790dc97c396 Mon Sep 17 00:00:00 2001 From: shashank-100 Date: Mon, 7 Sep 2026 10:43:07 +0530 Subject: [PATCH 3/4] fix(cli): rank an install's own skills above stale sibling trees --- packages/hunk/src/core/run/paths.test.ts | 58 ++++++++++++++++++++---- packages/hunk/src/core/run/paths.ts | 19 +++++--- 2 files changed, 60 insertions(+), 17 deletions(-) diff --git a/packages/hunk/src/core/run/paths.test.ts b/packages/hunk/src/core/run/paths.test.ts index 80537c5ae..769a851bc 100644 --- a/packages/hunk/src/core/run/paths.test.ts +++ b/packages/hunk/src/core/run/paths.test.ts @@ -120,13 +120,17 @@ describe("paths", () => { } }); - test("prefers the packaged layout over a generic skills directory beside it", () => { + test("prefers an install's own skills over both staging trees beside it", () => { const tempRoot = createTempRoot("hunk-skill-specificity-"); try { - // Both shapes at one ancestor: the one naming Hunk explicitly is the bundled skill, - // and the bare `skills/` sibling belongs to whatever else lives in that directory. - const packagedSkill = join( + // All three shapes at one ancestor. The official installer writes `skills/` beside the + // binary while a source install stages `hunkdiff/skills/`, and neither removes the + // other's tree — so reinstalling one way over the other leaves a stale sibling that must + // not win. `node_modules/hunkdiff` belongs to whatever project shares the directory. + const installedSkill = join(tempRoot, "skills", "hunk-review", "SKILL.md"); + const staleStagedSkill = join(tempRoot, "hunkdiff", "skills", "hunk-review", "SKILL.md"); + const staleNestedSkill = join( tempRoot, "node_modules", "hunkdiff", @@ -134,16 +138,50 @@ describe("paths", () => { "hunk-review", "SKILL.md", ); - const siblingSkill = join(tempRoot, "skills", "hunk-review", "SKILL.md"); const fakeBinary = join(tempRoot, "hunk"); - mkdirSync(dirname(packagedSkill), { recursive: true }); - mkdirSync(dirname(siblingSkill), { recursive: true }); - writeFileSync(packagedSkill, "# packaged\n"); - writeFileSync(siblingSkill, "# sibling\n"); + for (const skill of [installedSkill, staleStagedSkill, staleNestedSkill]) { + mkdirSync(dirname(skill), { recursive: true }); + } + writeFileSync(installedSkill, "# installed\n"); + writeFileSync(staleStagedSkill, "# stale staging\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(packagedSkill); + expect(resolveBundledSkillPath("hunk-review", [fakeBinary])).toBe(shippedSkill); } finally { rmSync(tempRoot, { recursive: true, force: true }); } diff --git a/packages/hunk/src/core/run/paths.ts b/packages/hunk/src/core/run/paths.ts index c97b61e12..4fb4af3db 100644 --- a/packages/hunk/src/core/run/paths.ts +++ b/packages/hunk/src/core/run/paths.ts @@ -156,8 +156,11 @@ function findRelativePathFromAncestors(startPath: string, relativePaths: readonl /** * 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. What differs is the prefix each + * layout puts in front of it, and their relative order is load-bearing: an install's own + * `skills/` must outrank both a leftover `hunkdiff/skills/` staging tree and a + * `node_modules/hunkdiff` belonging to some other project, or a stale copy wins. */ export function resolveBundledSkillPath( name: BundledSkillName = DEFAULT_BUNDLED_SKILL_NAME, @@ -165,13 +168,15 @@ export function resolveBundledSkillPath( ) { const roots = searchRoots ?? [import.meta.dir, process.execPath]; const skillRelativePath = join("skills", name, "SKILL.md"); - // Most specific first, so a directory holding several layouts resolves to the one that - // names Hunk explicitly. A source install stages its skills under `hunkdiff/` beside the - // executable precisely to avoid claiming the generic `skills/` name in a bin directory. + // Order within one directory, own-copy first. Both installers write their skills beside the + // binary — the official one to `skills/`, a source install to `hunkdiff/skills/` — and neither + // removes the other's tree, so `skills/` leads or a leftover source staging tree serves a + // newer install's skills. `node_modules/hunkdiff` is last either way: it belongs to whatever + // project shares the directory and may be pinned to another version. const relativeCandidates = [ - join("node_modules", "hunkdiff", skillRelativePath), - join("hunkdiff", skillRelativePath), skillRelativePath, + join("hunkdiff", skillRelativePath), + join("node_modules", "hunkdiff", skillRelativePath), ]; for (const root of roots) { From fd3928b1b8e24439e5a7d1865d3e7427f344fd69 Mon Sep 17 00:00:00 2001 From: shashank-100 Date: Mon, 7 Sep 2026 10:58:02 +0530 Subject: [PATCH 4/4] fix(cli): prefer staged skills before generic sibling copies --- .changeset/staged-skill-resolution-proximity.md | 2 +- packages/hunk/src/core/run/paths.test.ts | 15 ++++++--------- packages/hunk/src/core/run/paths.ts | 15 +++++---------- 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/.changeset/staged-skill-resolution-proximity.md b/.changeset/staged-skill-resolution-proximity.md index 9cbf80561..7abe287f1 100644 --- a/.changeset/staged-skill-resolution-proximity.md +++ b/.changeset/staged-skill-resolution-proximity.md @@ -2,4 +2,4 @@ "hunkdiff": patch --- -Stop an unrelated `skills/` directory above the install location from shadowing the bundled skill a source install staged beside the Hunk executable. +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 769a851bc..7ff74b478 100644 --- a/packages/hunk/src/core/run/paths.test.ts +++ b/packages/hunk/src/core/run/paths.test.ts @@ -120,16 +120,13 @@ describe("paths", () => { } }); - test("prefers an install's own skills over both staging trees beside it", () => { + 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 official installer writes `skills/` beside the - // binary while a source install stages `hunkdiff/skills/`, and neither removes the - // other's tree — so reinstalling one way over the other leaves a stale sibling that must - // not win. `node_modules/hunkdiff` belongs to whatever project shares the directory. - const installedSkill = join(tempRoot, "skills", "hunk-review", "SKILL.md"); - const staleStagedSkill = join(tempRoot, "hunkdiff", "skills", "hunk-review", "SKILL.md"); + // 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", @@ -140,11 +137,11 @@ describe("paths", () => { ); const fakeBinary = join(tempRoot, "hunk"); - for (const skill of [installedSkill, staleStagedSkill, staleNestedSkill]) { + for (const skill of [installedSkill, staleGenericSkill, staleNestedSkill]) { mkdirSync(dirname(skill), { recursive: true }); } writeFileSync(installedSkill, "# installed\n"); - writeFileSync(staleStagedSkill, "# stale staging\n"); + writeFileSync(staleGenericSkill, "# stale generic\n"); writeFileSync(staleNestedSkill, "# stale nested\n"); writeFileSync(fakeBinary, "binary\n"); diff --git a/packages/hunk/src/core/run/paths.ts b/packages/hunk/src/core/run/paths.ts index 4fb4af3db..ec9f65f12 100644 --- a/packages/hunk/src/core/run/paths.ts +++ b/packages/hunk/src/core/run/paths.ts @@ -157,10 +157,8 @@ function findRelativePathFromAncestors(startPath: string, relativePaths: readonl * 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 stays one walk. What differs is the prefix each - * layout puts in front of it, and their relative order is load-bearing: an install's own - * `skills/` must outrank both a leftover `hunkdiff/skills/` staging tree and a - * `node_modules/hunkdiff` belonging to some other project, or a stale copy wins. + * 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, @@ -168,14 +166,11 @@ export function resolveBundledSkillPath( ) { const roots = searchRoots ?? [import.meta.dir, process.execPath]; const skillRelativePath = join("skills", name, "SKILL.md"); - // Order within one directory, own-copy first. Both installers write their skills beside the - // binary — the official one to `skills/`, a source install to `hunkdiff/skills/` — and neither - // removes the other's tree, so `skills/` leads or a leftover source staging tree serves a - // newer install's skills. `node_modules/hunkdiff` is last either way: it belongs to whatever - // project shares the directory and may be pinned to another version. + // 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), ];