diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f65ae2..067ae1b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 60a9341..c25127f 100644 --- a/README.md +++ b/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. diff --git a/src/dmx/loop_tools.py b/src/dmx/loop_tools.py index d221eca..8b597be 100644 --- a/src/dmx/loop_tools.py +++ b/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: diff --git a/tests/test_loop_tools.py b/tests/test_loop_tools.py index 9284b6e..e64d27c 100644 --- a/tests/test_loop_tools.py +++ b/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: