diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index 15304a8a9..02eb5224f 100644 --- a/doc/changes/unreleased.md +++ b/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. diff --git a/exasol/toolbox/util/skills.py b/exasol/toolbox/util/skills.py index 6eae1ff0d..55ec3ce2e 100644 --- a/exasol/toolbox/util/skills.py +++ b/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 diff --git a/test/unit/util/skill_test.py b/test/unit/util/skill_test.py index 0ccadbb28..8716a661c 100644 --- a/test/unit/util/skill_test.py +++ b/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,6 +156,34 @@ 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 _: {}) @@ -173,38 +191,31 @@ def test_install_skill_rejects_missing_skill(tmp_path, monkeypatch): 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")