fix(skills): support folder-shaped SKILL.md in .dmx/skills/ and bundled skills

CHANGELOG.md

@@ -9,6 +9,11 @@ Versioning follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
+### Fixed
+
+- **(GH-40)** The folder-shaped `{name}/SKILL.md` fallback (GH-27 phase 4) now applies to every skill-resolution tier, not just shared sources: a project's own `.dmx/skills/` and dmx's bundled `skills/` directory both get the same flat-then-folder-shaped lookup. Previously, a folder-shaped skill dropped straight into `.dmx/skills/` (e.g. by copying an agentskills.io/Claude Code skill in without going through a shared source) silently failed to resolve — `get_skill_definition` returned "Skill not found" with no indication a folder existed. Extracted the flat-then-folder lookup into a shared helper (`_find_skill_in_dir`) used by all three tiers; existing precedence (app repo > shared sources > bundled) and `root_path` semantics are unchanged.
+- **(GH-40 review)** `get_skill_definition`'s `name` argument is now validated as a plain slug (`_SKILL_NAME_RE`, mirroring `shared_sources.py`'s existing `name`/`subdir` validation) before any filesystem lookup. Found during review of the fix above: a `../`-laden or absolute skill name previously resolved to arbitrary files outside the intended `.dmx/skills/`, shared-source, or bundled directories — reproducible on `main` prior to this release, not introduced by the fix above, but widened from one reachable tier to three by it. A malformed name now behaves exactly like "skill not found" rather than reading the file.
+
 ## [0.4.1] — 2026-09-11
 
 ### Fixed

README.md

@@ -95,7 +95,7 @@ Then run `/dmx/sync` to clone each source at its pinned ref and vendor it into `
 - Fails clearly (not silently) if a source can't be cloned, its ref doesn't exist, the resolved directory doesn't look like a dmx shared source (no `loops/`, `skills/`, or `validators/` at the resolved path), or an entry under its `skills/` doesn't match either supported shape.
 - Warns — without failing the sync — about same-name collisions across the app repo and every declared source, e.g. two sources both defining `spec.yaml`, so an unintended shadow never goes unnoticed.
 
-Skills support two shapes inside a shared source's `skills/` directory: dmx's own flat `{name}.md`, or the [agentskills.io](https://agentskills.io) / Claude Code / Cursor convention of a `{name}/SKILL.md` folder with optional `scripts/`, `references/`, and `assets/` — so a shared source can point straight at an org's existing standards-shaped skills repo with zero dmx-specific restructuring. When a folder-shaped skill resolves, dmx tells the agent its on-disk root path so it can resolve those `scripts/`/`references/`/`assets/` paths directly, and surfaces any `dependencies:` declared in its frontmatter as an explicit note (dmx has no auto-install step of its own).
+Skills support two shapes, in every tier that resolves them (a project's own `.dmx/skills/`, a shared source's `skills/`, and dmx's bundled skills): dmx's own flat `{name}.md`, or the [agentskills.io](https://agentskills.io) / Claude Code / Cursor convention of a `{name}/SKILL.md` folder with optional `scripts/`, `references/`, and `assets/` — so a shared source (or a project directly) can point straight at an org's existing standards-shaped skills repo with zero dmx-specific restructuring. When a folder-shaped skill resolves, dmx tells the agent its on-disk root path so it can resolve those `scripts/`/`references/`/`assets/` paths directly, and surfaces any `dependencies:` declared in its frontmatter as an explicit note (dmx has no auto-install step of its own).
 
 Because everything is vendored and committed rather than fetched live at resolve time, network and git credentials are only needed at `/dmx/sync` time — infrequent and human-reviewed — not on every loop run. `/dmx/sync` shells out to plain `git`, so it inherits whatever credentials the invoking environment's git already has (SSH agent, `gh auth`, a CI deploy key or machine-user PAT) — nothing dmx-specific to configure, and it works the same way against GitHub, GitHub Enterprise, GitLab, or any other git host. See [#27](https://github.com/deepmodel-ai/dmx/issues/27) for the full design.
 

src/dmx/loop_tools.py

@@ -90,6 +90,15 @@ def _bundled_skills_dir() -> Path:
     return Path(str(pkg.files("dmx") / "skills"))
 
 
+# `name` (and the shared-source config's own `name`/`subdir` fields — see
+# shared_sources._NAME_RE) becomes a literal path segment below `.dmx/skills/`,
+# a shared source's `skills/`, or the bundled skills dir. Restricting it to a
+# plain slug (no `/`, no `..`, no leading `-`) rules out escaping those
+# directories via a `../`-laden or absolute skill name passed to
+# `get_skill_definition` — see GH-40's review for the concrete reproduction.
+_SKILL_NAME_RE = re.compile(r"^[a-zA-Z0-9_][a-zA-Z0-9_-]*$")
+
+
 @dataclass(frozen=True)
 class ResolvedSkill:
     """A skill found by :func:`_resolve_skill`.
@@ -110,58 +119,103 @@ class ResolvedSkill:
     root_path: str | None = None
 
 
+def _find_skill_in_dir(
+    skills_dir: Path,
+    candidates: list[str],
+    workspace_root: Path,
+    *,
+    recursive: bool,
+) -> ResolvedSkill | None:
+    """Look up *candidates* under *skills_dir*: dmx's flat ``{name}.md``
+    first, then the folder-shaped ``{name}/SKILL.md`` convention (the
+    agentskills.io / Claude Code ecosystem shape, with optional
+    ``scripts/``/``references/``/``assets/`` — see GH-27 phase 4) as a
+    fallback. Checked in this order for every candidate before moving on —
+    flat always wins over folder-shaped for the same logical name.
+
+    Args:
+        recursive: ``True`` to search anywhere under *skills_dir* (needed
+            for the bundled skills directory, which nests skills under
+            category subdirectories, e.g. ``workflow/0-init/dmx-init.md``).
+            ``False`` to only look directly inside *skills_dir* (the flat,
+            single-level convention used by ``.dmx/skills/`` and every
+            shared source's ``skills/`` directory).
+    """
+    for candidate in candidates:
+        if recursive:
+            matches = list(skills_dir.rglob(f"{candidate}.md"))
+            if matches:
+                return ResolvedSkill(raw=matches[0].read_text())
+        else:
+            path = skills_dir / f"{candidate}.md"
+            if path.exists():
+                return ResolvedSkill(raw=path.read_text())
+
+    for candidate in candidates:
+        if recursive:
+            matches = list(skills_dir.rglob(f"{candidate}/SKILL.md"))
+            if not matches:
+                continue
+            path = matches[0]
+            skill_dir = path.parent
+        else:
+            skill_dir = skills_dir / candidate
+            path = skill_dir / "SKILL.md"
+            if not path.exists():
+                continue
+        try:
+            root_path = str(skill_dir.relative_to(workspace_root))
+        except ValueError:
+            # Outside workspace_root — only possible for the bundled
+            # skills directory (installed with the package, not vendored
+            # into the workspace). Fall back to an absolute path so the
+            # agent still has something resolvable.
+            root_path = str(skill_dir)
+        return ResolvedSkill(raw=path.read_text(), root_path=root_path)
+
+    return None
+
+
 def _resolve_skill(name: str, workspace_root: Path) -> ResolvedSkill | None:
     """Find a skill by name.
 
-    Search order:
-    1. ``{workspace_root}/.dmx/skills/{name}.md`` (project-specific, exact)
-    2. ``{workspace_root}/.dmx/skills/dmx-{name}.md`` (project-specific, prefixed)
-    3. For each declared ``shared_sources`` entry, in declared order:
-       a. ``.dmx/vendor/{source}/skills/{name}.md`` (or ``dmx-{name}.md``) —
-          dmx's own flat convention, checked first (cheap, and matches how
-          dmx already writes skills everywhere else).
-       b. ``.dmx/vendor/{source}/skills/{name}/SKILL.md`` (or
-          ``dmx-{name}/SKILL.md``) — the agentskills.io / Claude Code
-          ecosystem convention, as a fallback, so an org can point a shared
-          source directly at an already-standards-shaped skills repo with
-          zero dmx-specific restructuring. See GH-27.
-    4. Recursive glob in the bundled skills directory for ``{name}.md``
-    5. Recursive glob in the bundled skills directory for ``dmx-{name}.md``
-
-    Only step 3b ever sets :attr:`ResolvedSkill.root_path` — the flat form
-    (steps 1, 2, 3a, 4, 5) never needs it.
-
-    Returns ``None`` if the skill is not found in any location.
+    Search order — each tier tries the flat ``{name}.md``/``dmx-{name}.md``
+    form first, then the folder-shaped ``{name}/SKILL.md`` form as a
+    fallback (see :func:`_find_skill_in_dir`):
+
+    1. ``{workspace_root}/.dmx/skills/`` (project-specific override)
+    2. For each declared ``shared_sources`` entry, in declared order:
+       ``.dmx/vendor/{source}/skills/``
+    3. The bundled ``skills/`` directory shipped with dmx
+
+    ``root_path`` is set on the result whenever the folder-shaped form
+    matched, regardless of which tier — a skill needs it any time it has
+    its own directory for ``scripts/``/``references/``/``assets/`` to
+    resolve against, not just when it came from a shared source.
+
+    Returns ``None`` if the skill is not found in any location, or if
+    *name* isn't a plain slug (see ``_SKILL_NAME_RE``) — a `/`, `..`, or
+    absolute-path-shaped name is never a real skill, only ever a path
+    traversal attempt, so it's rejected before touching the filesystem.
     """
+    if not _SKILL_NAME_RE.match(name):
+        return None
+
     candidates = [name, f"dmx-{name}"]
 
     project_skills = workspace_root / ".dmx" / "skills"
-    for candidate in candidates:
-        path = project_skills / f"{candidate}.md"
-        if path.exists():
-            return ResolvedSkill(raw=path.read_text())
+    resolved = _find_skill_in_dir(project_skills, candidates, workspace_root, recursive=False)
+    if resolved is not None:
+        return resolved
 
     for source in read_shared_sources(workspace_root):
         source_skills = source_root(workspace_root, source) / "skills"
-        for candidate in candidates:
-            path = source_skills / f"{candidate}.md"
-            if path.exists():
-                return ResolvedSkill(raw=path.read_text())
-        for candidate in candidates:
-            skill_dir = source_skills / candidate
-            path = skill_dir / "SKILL.md"
-            if path.exists():
-                return ResolvedSkill(
-                    raw=path.read_text(), root_path=str(skill_dir.relative_to(workspace_root))
-                )
+        resolved = _find_skill_in_dir(source_skills, candidates, workspace_root, recursive=False)
+        if resolved is not None:
+            return resolved
 
     bundled = _bundled_skills_dir()
-    for candidate in candidates:
-        matches = list(bundled.rglob(f"{candidate}.md"))
-        if matches:
-            return ResolvedSkill(raw=matches[0].read_text())
-
-    return None
+    return _find_skill_in_dir(bundled, candidates, workspace_root, recursive=True)
 
 
 def _dependencies_note(raw: str) -> str | None:

tests/test_loop_tools.py

@@ -912,6 +912,50 @@ def test_not_found_returns_none(self, tmp_path: Path) -> None:
         _write_shared_sources_config(tmp_path, [("acme", "git::https://x//?ref=v1")])
         assert _resolve_skill("totally-nonexistent-skill", tmp_path) is None
 
+    def test_path_traversal_name_rejected_even_when_target_exists(self, tmp_path: Path) -> None:
+        """GH-40 review: a `../`-laden name must never escape .dmx/skills/,
+        a shared source's skills/, or the bundled skills dir — reject it
+        outright rather than letting it resolve to a real file elsewhere."""
+        workspace_root = tmp_path / "workspace"
+        (workspace_root / ".dmx" / "skills").mkdir(parents=True)
+        outside = tmp_path / "outside-workspace"
+        outside.mkdir()
+        (outside / "evil.md").write_text("# should never resolve\n", encoding="utf-8")
+
+        assert _resolve_skill("../outside-workspace/evil", workspace_root) is None
+
+    def test_folder_shaped_path_traversal_name_rejected(self, tmp_path: Path) -> None:
+        workspace_root = tmp_path / "workspace"
+        (workspace_root / ".dmx" / "skills").mkdir(parents=True)
+        outside = tmp_path / "outside-workspace"
+        outside.mkdir()
+        (outside / "SKILL.md").write_text("# should never resolve\n", encoding="utf-8")
+
+        assert _resolve_skill("../outside-workspace", workspace_root) is None
+
+    def test_absolute_path_name_rejected(self, tmp_path: Path) -> None:
+        workspace_root = tmp_path / "workspace"
+        (workspace_root / ".dmx" / "skills").mkdir(parents=True)
+        assert _resolve_skill("/etc/passwd", workspace_root) is None
+
+    def test_leading_hyphen_name_rejected(self, tmp_path: Path) -> None:
+        # Mirrors shared_sources._NAME_RE's own restriction — a leading
+        # "-" could be misread as a flag by anything that later shells out
+        # using this name; reject it here too, defensively.
+        workspace_root = tmp_path / "workspace"
+        (workspace_root / ".dmx" / "skills").mkdir(parents=True)
+        assert _resolve_skill("-rf", workspace_root) is None
+
+    def test_ordinary_hyphenated_skill_name_still_resolves(self, tmp_path: Path) -> None:
+        # Confirm the guard doesn't collaterally break real skill names,
+        # which are routinely hyphenated (e.g. "create-ticket").
+        skill_path = tmp_path / ".dmx" / "skills" / "my-real-skill.md"
+        skill_path.parent.mkdir(parents=True, exist_ok=True)
+        skill_path.write_text("# real skill\n", encoding="utf-8")
+        resolved = _resolve_skill("my-real-skill", tmp_path)
+        assert resolved is not None
+        assert resolved.raw == "# real skill\n"
+
     def test_folder_shaped_skill_found_as_fallback_after_flat(self, tmp_path: Path) -> None:
         _write_shared_sources_config(tmp_path, [("acme", "git::https://x//?ref=v1")])
         skill_dir = tmp_path / ".dmx" / "vendor" / "acme" / "skills" / "custom-skill"
@@ -951,19 +995,99 @@ def test_dmx_prefixed_folder_shaped_skill_is_found(self, tmp_path: Path) -> None
         assert resolved.raw == "# dmx-prefixed folder-shaped\n"
         assert resolved.root_path == ".dmx/vendor/acme/skills/dmx-custom-skill"
 
-    def test_folder_shaped_form_is_shared_source_only_not_app_repo_or_bundled(
+    def test_folder_shaped_skill_resolves_from_app_repos_own_dmx_skills(
+        self, tmp_path: Path
+    ) -> None:
+        """GH-40: the folder-shaped form isn't shared-source-only — a
+        project's own .dmx/skills/ gets the same fallback."""
+        skill_dir = tmp_path / ".dmx" / "skills" / "custom-skill"
+        skill_dir.mkdir(parents=True, exist_ok=True)
+        (skill_dir / "SKILL.md").write_text("# app repo folder-shaped\n", encoding="utf-8")
+
+        resolved = _resolve_skill("custom-skill", tmp_path)
+
+        assert resolved is not None
+        assert resolved.raw == "# app repo folder-shaped\n"
+        assert resolved.root_path == ".dmx/skills/custom-skill"
+
+    def test_dmx_prefixed_folder_shaped_skill_resolves_from_app_repo(self, tmp_path: Path) -> None:
+        skill_dir = tmp_path / ".dmx" / "skills" / "dmx-custom-skill"
+        skill_dir.mkdir(parents=True, exist_ok=True)
+        (skill_dir / "SKILL.md").write_text("# app repo dmx-prefixed\n", encoding="utf-8")
+
+        resolved = _resolve_skill("custom-skill", tmp_path)
+
+        assert resolved is not None
+        assert resolved.raw == "# app repo dmx-prefixed\n"
+        assert resolved.root_path == ".dmx/skills/dmx-custom-skill"
+
+    def test_app_repo_flat_form_still_beats_app_repo_folder_shaped_form(
         self, tmp_path: Path
     ) -> None:
-        # A folder named "custom-skill/" with a SKILL.md sitting under the
-        # app repo's own .dmx/skills/ isn't part of this phase's scope — the
-        # app-repo tier only ever checked flat {name}.md files, and phase 4
-        # doesn't change that. Confirm it's simply not found (not a crash,
-        # not misresolved).
+        flat_path = tmp_path / ".dmx" / "skills" / "custom-skill.md"
+        flat_path.parent.mkdir(parents=True, exist_ok=True)
+        flat_path.write_text("# app flat\n", encoding="utf-8")
         skill_dir = tmp_path / ".dmx" / "skills" / "custom-skill"
         skill_dir.mkdir(parents=True, exist_ok=True)
-        (skill_dir / "SKILL.md").write_text("# should not resolve\n", encoding="utf-8")
+        (skill_dir / "SKILL.md").write_text("# app folder-shaped\n", encoding="utf-8")
+
+        resolved = _resolve_skill("custom-skill", tmp_path)
+
+        assert resolved is not None
+        assert resolved.raw == "# app flat\n"
+        assert resolved.root_path is None
 
-        assert _resolve_skill("custom-skill", tmp_path) is None
+    def test_app_repo_folder_shaped_beats_shared_source_and_bundled(self, tmp_path: Path) -> None:
+        """Tier precedence (app repo > shared sources > bundled) still
+        holds when the app repo's match is folder-shaped."""
+        _write_shared_sources_config(tmp_path, [("acme", "git::https://x//?ref=v1")])
+        shared_path = tmp_path / ".dmx" / "vendor" / "acme" / "skills" / "custom-skill.md"
+        shared_path.parent.mkdir(parents=True, exist_ok=True)
+        shared_path.write_text("# shared flat\n", encoding="utf-8")
+        skill_dir = tmp_path / ".dmx" / "skills" / "custom-skill"
+        skill_dir.mkdir(parents=True, exist_ok=True)
+        (skill_dir / "SKILL.md").write_text("# app folder-shaped\n", encoding="utf-8")
+
+        resolved = _resolve_skill("custom-skill", tmp_path)
+
+        assert resolved is not None
+        assert resolved.raw == "# app folder-shaped\n"
+        assert resolved.root_path == ".dmx/skills/custom-skill"
+
+    def test_folder_shaped_bundled_skill_resolves_with_root_path(
+        self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """GH-40: the bundled tier also gets the folder-shaped fallback —
+        including nested category subdirectories, matching how bundled
+        skills are actually laid out (e.g. workflow/0-init/dmx-init.md)."""
+        bundled_dir = tmp_path / "bundled-skills"
+        skill_dir = bundled_dir / "utility" / "custom-bundled-skill"
+        skill_dir.mkdir(parents=True, exist_ok=True)
+        (skill_dir / "SKILL.md").write_text("# bundled folder-shaped\n", encoding="utf-8")
+        monkeypatch.setattr("dmx.loop_tools._bundled_skills_dir", lambda: bundled_dir)
+
+        workspace_root = tmp_path / "workspace"
+        workspace_root.mkdir()
+        resolved = _resolve_skill("custom-bundled-skill", workspace_root)
+
+        assert resolved is not None
+        assert resolved.raw == "# bundled folder-shaped\n"
+        # skill_dir is outside workspace_root — falls back to an absolute path.
+        assert resolved.root_path == str(skill_dir)
+
+    def test_folder_shaped_bundled_skill_not_shadowed_by_app_repo_or_shared_source(
+        self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        bundled_dir = tmp_path / "bundled-skills"
+        skill_dir = bundled_dir / "custom-bundled-only"
+        skill_dir.mkdir(parents=True, exist_ok=True)
+        (skill_dir / "SKILL.md").write_text("# bundled only\n", encoding="utf-8")
+        monkeypatch.setattr("dmx.loop_tools._bundled_skills_dir", lambda: bundled_dir)
+
+        resolved = _resolve_skill("custom-bundled-only", tmp_path)
+
+        assert resolved is not None
+        assert resolved.raw == "# bundled only\n"
 
 
 class TestDependenciesNote: