Refactor skill installation helpers

doc/changes/unreleased.md

@@ -12,3 +12,4 @@
 
 * #924: Removed the deprecated `lint:dependencies` Nox session and its unused implementation and tests.
 * #972: Switched from ravsamhq/notify-slack-action to maintained slackapi/slack-github-action & removed workflow versioned header
+* #967: Refactored skill installation into reusable filesystem helpers.

exasol/toolbox/util/skills.py

@@ -71,20 +71,17 @@ def _has_symlink_in_parents(path: Path) -> bool:
     return any(candidate.is_symlink() for candidate in (path, *path.parents))
 
 
-def install_skill(
-    skill_name: str = PTB_SKILL_NAME,
-    target_directory: Path | None = None,
-) -> Path:
-    """Install a packaged skill into a project-local agent skill directory."""
+def _validate_skill_name(skill_name: str) -> None:
+    """Reject skill names that could escape the skill installation directory."""
+    # Validate before joining the name with the destination path.
     if Path(skill_name).name != skill_name:
         raise ValueError(f"invalid skill name: {skill_name}")
 
-    source_files = get_skill_files(skill_name)
-    if not source_files:
-        raise ValueError(f"packaged skill does not exist: {skill_name}")
 
-    target_directory = target_directory or Path.cwd() / ".agents" / "skills"
+def _prepare_installation_directory(target_directory: Path, skill_name: str) -> Path:
+    """Validate and recreate the destination directory for one skill."""
     target_skill = target_directory / skill_name
+    # Do not follow symlinks when creating or replacing an installation.
     if _has_symlink_in_parents(target_directory):
         raise ValueError(
             f"refusing to use symlinked target directory: {target_directory}"
@@ -94,13 +91,36 @@ def install_skill(
     if target_skill.exists() and not target_skill.is_dir():
         raise ValueError(f"skill target is not a directory: {target_skill}")
 
+    # Replace an old installation only after validating its type and symlink state.
     if target_skill.exists():
         shutil.rmtree(target_skill)
     target_skill.mkdir(parents=True, exist_ok=True)
+    return target_skill
+
+
+def _copy_skill_files(
+    source_files: Mapping[str, Traversable], target_directory: Path
+) -> None:
+    """Copy packaged skill files below an already validated target directory."""
     for relative_path, source in source_files.items():
-        destination = target_skill / relative_path
+        destination = target_directory / relative_path
         destination.parent.mkdir(parents=True, exist_ok=True)
         destination.write_bytes(source.read_bytes())
+
+
+def install_skill(
+    skill_name: str = PTB_SKILL_NAME,
+    target_directory: Path | None = None,
+) -> Path:
+    """Install a packaged skill into a project-local agent skill directory."""
+    _validate_skill_name(skill_name)
+    source_files = get_skill_files(skill_name)
+    if not source_files:
+        raise ValueError(f"packaged skill does not exist: {skill_name}")
+
+    target_directory = target_directory or Path.cwd() / ".agents" / "skills"
+    target_skill = _prepare_installation_directory(target_directory, skill_name)
+    _copy_skill_files(source_files, target_skill)
     return target_skill
 
 

test/unit/util/skill_test.py

@@ -3,21 +3,11 @@
 
 from exasol.toolbox.util import skills
 from exasol.toolbox.util.skill_eval import PackagedSkillEvalCases
-
-
-@pytest.fixture
-def mock_packaged_skill_files(tmp_path, monkeypatch):
-    source = tmp_path / "source"
-    source.mkdir()
-
-    skill_file = source / "SKILL.md"
-    skill_file.write_text("skill", encoding="utf-8")
-
-    monkeypatch.setattr(
-        skills,
-        "get_skill_files",
-        lambda _: {"SKILL.md": skill_file},
-    )
+from exasol.toolbox.util.skills import (
+    _copy_skill_files,
+    _prepare_installation_directory,
+    _validate_skill_name,
+)
 
 
 def test_validate_skill_accepts_packaged_ptb_skill():
@@ -166,45 +156,66 @@ def test_install_skill_rejects_path_traversal(tmp_path):
         skills.install_skill("../outside", tmp_path)
 
 
+def test_validate_skill_name_rejects_path_traversal():
+    with pytest.raises(ValueError, match="invalid skill name"):
+        _validate_skill_name("nested/example")
+
+
+def test_prepare_installation_directory_replaces_existing_directory(tmp_path):
+    target_directory = tmp_path / ".agents" / "skills"
+    target_skill = target_directory / "example"
+    target_skill.mkdir(parents=True)
+    (target_skill / "stale.md").write_text("stale", encoding="utf-8")
+
+    prepared = _prepare_installation_directory(target_directory, "example")
+
+    assert prepared == target_skill
+    assert not (target_skill / "stale.md").exists()
+
+
+def test_copy_skill_files_copies_nested_files(tmp_path):
+    source = tmp_path / "source.md"
+    source.write_text("content", encoding="utf-8")
+    target = tmp_path / "target"
+    target.mkdir()
+
+    _copy_skill_files({"references/source.md": source}, target)
+
+    assert (target / "references/source.md").read_text(encoding="utf-8") == "content"
+
+
 def test_install_skill_rejects_missing_skill(tmp_path, monkeypatch):
     monkeypatch.setattr(skills, "get_skill_files", lambda _: {})
 
     with pytest.raises(ValueError, match="packaged skill does not exist"):
         skills.install_skill("example", tmp_path)
 
 
-def test_install_skill_rejects_symlinked_target_directory(
-    tmp_path, mock_packaged_skill_files
-):
+def test_prepare_installation_directory_rejects_symlinked_target_directory(tmp_path):
     real_target = tmp_path / "real-target"
     real_target.mkdir()
     target_directory = tmp_path / "linked-target"
     target_directory.symlink_to(real_target, target_is_directory=True)
 
     with pytest.raises(ValueError, match="symlinked target directory"):
-        skills.install_skill("example", target_directory)
+        _prepare_installation_directory(target_directory, "example")
 
 
-def test_install_skill_rejects_file_as_target(tmp_path, mock_packaged_skill_files):
+def test_prepare_installation_directory_rejects_file_as_target(tmp_path):
     target_directory = tmp_path / ".agents" / "skills"
     target_directory.mkdir(parents=True)
     (target_directory / "example").write_text("not a directory", encoding="utf-8")
 
     with pytest.raises(ValueError, match="not a directory"):
-        skills.install_skill("example", target_directory)
+        _prepare_installation_directory(target_directory, "example")
 
 
-def test_install_skill_rejects_symlink_target(tmp_path, monkeypatch):
-    source = tmp_path / "source"
-    source.mkdir()
-    skill_file = source / "SKILL.md"
-    skill_file.write_text("skill", encoding="utf-8")
-    monkeypatch.setattr(skills, "get_skill_files", lambda _: {"SKILL.md": skill_file})
+def test_prepare_installation_directory_rejects_symlink_target(tmp_path):
     target_directory = tmp_path / ".agents" / "skills"
     target_directory.mkdir(parents=True)
-    target = tmp_path / "elsewhere"
-    target.mkdir()
-    (target_directory / "example").symlink_to(target, target_is_directory=True)
+    symlink_target = tmp_path / "elsewhere"
+    symlink_target.mkdir()
+    (target_directory / "example").symlink_to(symlink_target, target_is_directory=True)
 
     with pytest.raises(ValueError, match="refusing to replace symlink"):
-        skills.install_skill("example", target_directory)
+        _prepare_installation_directory(target_directory, "example")