fix(cli): resolve bundled skills by proximity, not candidate order - #1007
Open
shashank-100 wants to merge 3 commits into
Open
fix(cli): resolve bundled skills by proximity, not candidate order#1007shashank-100 wants to merge 3 commits into
shashank-100 wants to merge 3 commits into
Conversation
`resolveBundledSkillPath` walked each candidate shape to the filesystem root before trying the next one, so the generic `skills/<name>/SKILL.md` shape was tested at every ancestor before `hunkdiff/skills/<name>/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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019jAgP7j1gaT9fSk5syLHDH
Contributor
|
PR author is not in the allowed authors list. |
|
@shashank-100 is attempting to deploy a commit to the Modem Team on Vercel. A member of the Team first needs to authorize it. |
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019jAgP7j1gaT9fSk5syLHDH
Member
|
Nice fix. The ancestor-first walk looks right. One small tweak: please prefer |
Order the candidate shapes within one directory as `hunkdiff/skills`, then `skills`, then `node_modules/hunkdiff/skills`. A standalone build ships its skills beside the executable, so that copy should win over a `node_modules/hunkdiff` belonging to whatever project happens to share the directory, which may be pinned to another version and left stale. Rework the specificity test to cover it: all three shapes at one ancestor, asserting the shipped copy resolves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019jAgP7j1gaT9fSk5syLHDH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
resolveBundledSkillPathnests its search loops candidate-outer, ancestor-inner, so each candidate shape is walked to the filesystem root before the next shape is tried at all:With
root = process.execPath(~/.local/bin/hunk), the genericskills/<name>/SKILL.mdshape is tested at~/.local/bin,~/.local,~,/home,/— all beforehunkdiff/skills/<name>/SKILL.mdis tried at~/.local/bin, wherescripts/install-bin.tsactually stages the skills for a source install.User impact: anyone with an unrelated
~/skills/hunk-review/SKILL.md— a personal skills folder, or a Hunk checkout at~— runsbun run install:binand then gets that unrelated file fromhunk skill path, not the one just installed.The
hunkdiff/nesting exists precisely to avoid claiming generic skill names beside the executable (see the comment atscripts/install-bin.ts:46-47); the resolver's loop order defeated that intent.Approach
Invert the nesting so every candidate shape is tested at one ancestor before the walk moves up — proximity wins over candidate order — and order the candidates most-specific-first so a directory holding several layouts resolves to the one that names Hunk explicitly.
findRelativePathFromAncestorsnow takes the candidate list rather than a single path. It has one caller, so no other resolution behavior changes.Non-goals: the set of supported layouts is unchanged, and no discovery root is added or removed. This only fixes which of several existing matches wins.
Why core
Skill resolution is CLI startup behavior in
core/run, not renderer or extension surface.Tests
Added two regression tests in
packages/hunk/src/core/run/paths.test.ts:skills/directory further up (the reported bug);node_modules/hunkdiff/layout beats a genericskills/sibling at the same ancestor.Both were confirmed to fail on the unfixed resolver and pass with the fix, so they guard the behavior rather than describe it. This path previously had no coverage for a competing ancestor.
Commands run
The 2 failures are in
test/cli/install-vm/contract.test.tsand are pre-existing onmain, not caused by this change. Reproduced on a clean tree atupstream/main:The harness compares a
realpathSync-resolved root against an unresolved target, so macOS's/var→/private/varsymlink makes the containment check fail. Unrelated to skill resolution.Platforms
Verified on macOS (darwin 25.6.0). Not run on Linux or Windows; the change is
node:path/node:fsonly and adds no platform assumptions, and the new tests build their fixtures withpath.joinrather than literal separators.Follow-ups
Not addressed here:
scripts/changeset-version.ts:54unconditionally deletes the staged package changelog in afinally, so a failed write-back on line 53 destroys the freshly generated changelog after the.changeset/*.mdfiles have already been consumed. Narrow, but recovery is manual and it runs during releases.🤖 Generated with Claude Code
https://claude.ai/code/session_019jAgP7j1gaT9fSk5syLHDH