fix(skills): support folder-shaped SKILL.md in .dmx/skills/ and bundled skills - #41
Merged
Merged
Conversation
…ed skills
Extend the folder-shaped {name}/SKILL.md fallback (GH-27 phase 4) beyond
shared sources to every skill-resolution tier: the app repo's own
.dmx/skills/ and dmx's bundled skills directory. Extracted the
flat-then-folder lookup into a shared helper, _find_skill_in_dir, reused
by all three tiers. Existing precedence (app repo > shared sources >
bundled) and root_path semantics are unchanged.
Fixes #40
Found in deep review of the folder-shaped-skill fix above: _resolve_skill built paths directly from get_skill_definition's name argument with no validation, so a '../'-laden or absolute name resolved to arbitrary files outside .dmx/skills/, a shared source's skills/, or the bundled skills dir. Reproducible on main before this PR (flat lookup already had it for all tiers; the folder-shaped form had it for shared sources); this PR's own fix widened the folder-shaped vector from one tier to three. Add _SKILL_NAME_RE, mirroring shared_sources.py's existing name/subdir slug validation, and reject non-slug names up front -- same behavior as 'skill not found', no filesystem access attempted. 6 new tests: traversal via flat form, via folder-shaped form, absolute path, leading-hyphen, and a sanity check that ordinary hyphenated skill names (the real-world norm) still resolve.
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
1 participant
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.
Fixes #40
Problem
The folder-shaped
{name}/SKILL.mdfallback added in GH-27 phase 4 only applied to the shared-source tier (.dmx/vendor/{source}/skills/). A skill dropped into a project's own.dmx/skills/in that same folder shape, or a bundled dmx skill shipped that way, silently failed to resolve —get_skill_definitionreturned a generic "Skill not found" with no hint that a matching folder existed.Fix
Extracted the flat-then-folder lookup into a shared helper,
_find_skill_in_dir(skills_dir, candidates, workspace_root, *, recursive), and reused it across all three tiers in_resolve_skill:.dmx/skills/(project override) — non-recursive, same as before..dmx/vendor/{source}/skills/(shared sources) — unchanged behavior, now sharing the helper.skills/— now recursive for both the flat and folder-shaped form, matching the bundled directory's actual nested category layout (e.g.workflow/0-init/dmx-init.md).root_pathis set whenever the folder-shaped form matches, in any tier. For the bundled tier specifically, the matched directory lives outsideworkspace_root(it's inside the installed package), soroot_pathfalls back to an absolute path in that case rather than raising.Existing precedence (app repo > shared sources > bundled) and root_path semantics for shared sources are unchanged — verified by the full existing
TestResolveSkillSharedSourcessuite passing unmodified except for one test that explicitly asserted the old (buggy) app-repo behavior, which is now flipped to assert the fix.Verification
mainin an isolated worktree: a folder-shaped skill in.dmx/skills/resolved toNone.root_path.ruff check,ruff format --check,mypy src/dmx,pytest -qall pass (491 tests).CHANGELOG.mdupdated under[Unreleased]. NoWORKFLOW_VERSIONbump — this is a bug fix, not a skill/rule/system-prompt change, per that constant's own bump policy.